dgram.send() callback can be executed either sync or async

[joeshaw]

There's no (good) way to tell if the callback you are passing into dgram.send() will be executed synchronously and recursively, or whether it'll be called async by the main loop. If you are expecting it to be deferred (as I was), you could easily hit a stack overflow. Consider the code:

// assume port & address are assigned
function sendChunk(socket, buffer, offset) {
    socket.send(buffer, offset, 1024, port, address, function(err) {
        if (err) throw err;

        offset += 1024;
        if (offset < buffer.length)
            sendChunk(socket, buffer, offset);
    });
}

With most other APIs in Node, the callback is executed in the main loop. With dgram.send() it's only async if a DNS lookup is required. Otherwise, it's inline and synchronous. In the code above, as long as address is an IP address and the buffer is large enough, it will crash with a stack overflow.

My buddy Havoc had a good blog post on this very topic a few days ago: http://blog.ometer.com/2011/07/24/callbacks-synchronous-and-asynchronous/ :)

[joeshaw]

As a broader question, is there a general convention in Node for whether callbacks are sync or async? I had generally assumed they were always async, and all the APIs I can think of that I used other than dgram.send() were. Is it just an aberration?

[joeshaw]

Looks like maybe a similar issue as to issue 1164 (#1164), and that would imply that sometimes a non-IP address value would also cause a sync callback.

[koichik]

I agree. I think dgram's methods should invoke callback asynchronously.
And, addMembership()/removeMembership() need callback...

[koichik]

Please review 09f753f.

[bnoordhuis]

@koichik: can we hold this off for a few days? I'm integrating libuv UDP support into node and this patch would mess up my work. libuv always executes the callback asynchronously, by the way.

[koichik]

@bnoordhuis

libuv always executes the callback asynchronously, by the way.

Awesome! I throw out this patch.

[joeshaw]

Probably still need the addMembership()/removeMembership() callbacks from the patch, though,