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

Upgrading caused promises to lose domain propagation #521

Closed
flankstaek opened this issue Mar 2, 2015 · 12 comments
Closed

Upgrading caused promises to lose domain propagation #521

flankstaek opened this issue Mar 2, 2015 · 12 comments

Comments

@flankstaek
Copy link

@flankstaek flankstaek commented Mar 2, 2015

We recently updated from version 2.3.4 to 2.9.12 and in doing so it broke domain propagation.

return platformHttp.get(platformUrl)
  .then(function () {
    return platformHttp.get(platformUrl);
  });

So within this code, our http handler accesses process.domain, within the first promise process.domain is defined and working correctly. But after the .then() process.domain no longer exists.

The only fix we could find was downgrading our installation to 2.8.2 because we needed the changes from there. I'm not sure which version broke it, but it was something between 2.8.2 and 2.9.12. Just thought I should let you all know, let me know if that wasn't clear or if you have any other questions.

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Mar 2, 2015

Domains are deprecated in io.js and support has been removed in 3.0. Not sure why you would use domains and promises at the same time though, promises make domains unnecessary.

@dillonkrug
Copy link

@dillonkrug dillonkrug commented Mar 2, 2015

I am also concerned about this. I use domains pretty heavily in my app, in conjunction with promises.

I use domains for request-specific data, like session:

// a domain is created for each incoming request.
SomeService.doTheThing = function (options) {
    return Promise.try(function(){
        var session = domain.active.session;
        // ... do stuff
    })
};

I am unsure as to how I would accomplish the same thing without domains.

I suppose it's possible to accomplish a similar effect with Promise.bind(), but then I can't use bind for it's intended purpose.

Is there something I'm missing?

@tyscorp
Copy link
Contributor

@tyscorp tyscorp commented Mar 2, 2015

You can use continuation-local-storage to achieve the same thing.

@dillonkrug
Copy link

@dillonkrug dillonkrug commented Mar 2, 2015

I actually was using cls originally, but it was a huge pain because everything had to be shimmed. Most of the libraries I use currently support domains without me having to do anything.

Asking a library author to support [Built In Library] seems pretty reasonable. I think it's much less likely that continuation-local-storage or whatever alternatives there may be will ever have the same support.

Io.js deprecating domains to get rid of the sketchy error handling seems like throwing the baby out with the bath water. An "official" api for context-specific variables is a very useful thing.

From the io.js docs on domains:

This module is pending deprecation. Once a replacement API has been finalized, this module will be fully deprecated. Most end users should not have cause to use this module.

It's obviously up to you @petkaantonov, but maybe it would be ok to keep domain support until the replacement API is released?

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Mar 3, 2015

@dillonkrug domains themselves are a hack that require shimming everything, the reason why it even worked with bluebird promises in the first place because I added hacky code for them. But they won't work with native promises at all for example, because they cannot be shimmed. So yes, node shims its standard libraries by default, and 3rd party modules mostly rely on those, so this probably isn't as visible to you - but it's a shimming hack alright.

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Apr 2, 2015

Fixed in 2.9.23. I will also retain domain feature in 3.0 - it turns out non-usage can be optimized completely.

petkaantonov added a commit that referenced this issue Apr 2, 2015
@johan
Copy link

@johan johan commented Apr 2, 2015

That release (2.9.23) is backwards incompatible with (at least) node v0.10.26, presumably on behalf of this change – do you think you could cater to both iojs and node, and/or bump the major version and unrelease 2.9.23?

.../node_modules/bluebird/js/main/async.js:75
        Object.defineProperty(EventsModule, "usingDomains", {
               ^
TypeError: Cannot redefine property: usingDomains
  at Function.defineProperty (native)
  at Object.<anonymous> (.../node_modules/bluebird/js/main/async.js:75:16)
  at Module._compile (module.js:456:26)
  at Object.Module._extensions..js (module.js:474:10)
  at Module.load (module.js:356:32)
  at Function.Module._load (module.js:312:12)
  at Module.require (module.js:364:17)
  at require (module.js:380:17)
  at module.exports (.../node_modules/bluebird/js/main/promise.js:13:13)
  at Object.<anonymous> (.../node_modules/bluebird/js/main/bluebird.js:9:39)

(I haven't tried 2.9.22 yet, but 2.9.21 still works just fine, so we can pin our dependencies to evade the problem temporarily at our end.)

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Apr 2, 2015

@johan This seems actually just a bug I didn't consider (multiple copies of bluebird being loaded). Are you loading multiple copies?

@johan
Copy link

@johan johan commented Apr 2, 2015

Confirmed; my package.json includes bluebird via two different packages (cached and gofer).

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Apr 2, 2015

Should be fixed in 2.9.24

@johan
Copy link

@johan johan commented Apr 2, 2015

Thanks! Verified working. ❤️

@jjofseattle
Copy link

@jjofseattle jjofseattle commented Nov 23, 2015

The bug seems to regress back in 2.10.2. Our domain code works well under 2.9.30, but finds corrupted domains with 2.10.2.

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

Successfully merging a pull request may close this issue.

None yet
6 participants