This repository has been archived by the owner. It is now read-only.

defer dgram listening event #3655

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants

dgram .bind's ._handle.lookup may be synchronous if bind is called without an address or if address is indeed an ip. This means 'listening' may fire before a caller has assigned a handler for the event.

This patch defers that event.

However if .bind is called with a dns name the event may be deferred a second tick since dns implicitly defers requests.

@tjfontaine tjfontaine referenced this pull request in tjfontaine/node-dns Jul 6, 2012

Closed

Serve on process.nextTick #19

Member

bnoordhuis commented Jul 6, 2012

I say it's a bug in .lookup() - callbacks are not supposed to run on the same tick to avoid infinite loop / stack overflow issues.

I don't disagree my initial gut said to change the behavior of lookup but then there's this comment https://github.com/joyent/node/blob/master/lib/dgram.js#L46

Member

bnoordhuis commented Jul 6, 2012

Yeah, I wrote that. In retrospect, that was probably a mistake - let's revisit it.

Just to be clear, you want to revisit the bind on send flow such that we can allow lookup to be async?

Some of the dgram tests work off the assumption that .bind is synchronous, for instance calling setTTL or addMembership immediately.

In reality they should be waiting for the listening event to ensure that the handle creation has completed. if we start enforcing these semantics now it will change the behavior for the simple case of .bind()

There's also one FIXME in the tests that indicates this is actually a deficiency in libuv, that one should be able to change membership or ttl without first calling bind, I don't agree with that assesment.

I've update the pullreq with what I think are the proper semantics

@bnoordhuis bnoordhuis commented on the diff Jul 9, 2012

lib/dgram.js
@@ -110,6 +105,23 @@ exports.createSocket = function(type, listener) {
return new Socket(type, listener);
};
+Socket.prototype._bind = function(ip, port) {
+ var ret;
+
+ if (!ip) {
+ if (this.type == 'udp4')
+ ip = '0.0.0.0';
+ else if (this.type == 'udp6')
+ ip = '::0';
@bnoordhuis

bnoordhuis Jul 9, 2012

Member
else
  assert(0); // or throw Error("unknown socket type")
Member

bnoordhuis commented Jul 9, 2012

@tjfontaine Maybe I'm missing something subtle here but isn't this patch all you need to make lookups always async?

diff --git a/lib/dgram.js b/lib/dgram.js
index 7abd0c3..4cfc6e6 100644
--- a/lib/dgram.js
+++ b/lib/dgram.js
@@ -43,11 +43,6 @@ function isIP(address) {


 function lookup(address, family, callback) {
-  // implicit 'bind before send' needs to run on the same tick
-  var matchedFamily = isIP(address);
-  if (matchedFamily)
-    return callback(null, address, matchedFamily);
-
   if (!dns)
     dns = require('dns');

Yes that was going to be my initial change, however I was trying to preserve the behavior such that implicit bind on send wouldn't delay another tick

With either approach though it changes when the handle actually gets the fd for things like setTTL to work, this should be the behavior people were expecting before but the tests are written contrary to that, does this mean the change won't make it into 0.8?

Member

bnoordhuis commented Jul 9, 2012

does this mean the change won't make it into 0.8?

Yes. Or rather - no, it won't. It's a change in behavior and therefore not eligible for inclusion in the stable branch.

It's not entirely a change anyone who calls .bind('foo.domain.com') or .bind('localhost') is already experiencing this behavior :)

Right after retesting the reason I couldn't just do that is throw new Error('implicit bind failed'); from https://github.com/joyent/node/blob/master/lib/dgram.js#L316-322 which is explicitly testing that things are created synchronously

Member

bnoordhuis commented Jul 9, 2012

Right, that code should go.

Member

bnoordhuis commented Jul 26, 2012

Half-merged in edd3de8. The issue's been addressed in 332fea5. Thanks, Timothy.

@bnoordhuis bnoordhuis closed this Jul 26, 2012

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.