Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Node domains only work when then-ing resolved promised #239

Closed
tp opened this issue Mar 31, 2014 · 9 comments
Closed

Node domains only work when then-ing resolved promised #239

tp opened this issue Mar 31, 2014 · 9 comments

Comments

@tp
Copy link

tp commented Mar 31, 2014

I am experiencing some issues around node domains and the timing of when then is called on a Promise.

Basically, all continuations created before the Promise is resolved loose their domain, and everything created after the Promise is resolved stay in the same domain.

var Promise = require('rsvp').Promise;
var request = require('request');

var p = new Promise(function(resolve, reject) {
  request('http://www.google.com', function(err, response, body) {
    console.log('done loading google.com')
    resolve(response.statusCode);
  });
});

var domain = require('domain');

var x = 0;

var f = function() {
  var id = ++x;
  console.log('%d waiting',id);

  var d = domain.create();

  process.domain = d;
  process.domain.x = 1;
  p.then(function(result) {
    console.log('%d continues',id);
    console.log('result = %d', result); 
    console.log('process.domain? %s', !! process.domain);
  });
};

f();
process.nextTick(f);
setTimeout(f, 100);
setTimeout(f, 1000); // set the timeout high, so that it surely fires after google.com has loaded

Output:

1 waiting
2 waiting
3 waiting
done loading google.com
1 continues
result = 200
process.domain? false
2 continues
result = 200
process.domain? false
3 continues
result = 200
process.domain? false
4 waiting
4 continues
result = 200
process.domain? true

As you can see, only the continuation that was created after the Promise had already been resolved works correctly.

@stefanpenner
Copy link
Collaborator

thanks for reporting this.

related solution on another library: petkaantonov/bluebird#152
Seems like implementing that here is the correct step

I should have time an evening this week, but if someone beats me to it, I'll be forever indebted.

@domenic
Copy link
Contributor

domenic commented Mar 31, 2014

There's a simpler solution, just call process.nextTick every time instead of saving in a var.

@stefanpenner
Copy link
Collaborator

@domenic orly?

@stefanpenner
Copy link
Collaborator

@domenic im a domain noob, but I don't think that is a problem as process.nextTick isn't being saved to a var, unless closing of process itself is the problem. I believe it's the optimization of 1 nextTick per run to completion.

@stefanpenner
Copy link
Collaborator

I believe I have a clear grasp on the issue and the solution. I'll likely through together a good set of tests, and work on this at some point when i have time.

@stefanpenner
Copy link
Collaborator

Sorry I haven't gotten to this yet, I will try to soon.

@stefanpenner
Copy link
Collaborator

as domains are stability 2 and the performance implications of doing this naively are non-trivial this is honestly pretty low priority for me.

@stefanpenner
Copy link
Collaborator

@domenic i'll implement this if you tell me i should (with good reason)

@domenic
Copy link
Contributor

domenic commented Sep 30, 2014

Nah


From: Stefan Pennermailto:notifications@github.com
Sent: ý2014-ý09-ý30 02:08
To: tildeio/rsvp.jsmailto:rsvp.js@noreply.github.com
Cc: Domenic Denicolamailto:domenic@domenicdenicola.com
Subject: Re: [rsvp.js] Node domains only work when then-ing resolved promised (#239)

@domenichttps://github.com/domenic i'll implement this if you tell me i should


Reply to this email directly or view it on GitHubhttps://github.com//issues/239#issuecomment-57253984.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants