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

Native promises + node.js domains #555

Closed
ORESoftware opened this issue Mar 29, 2017 · 11 comments
Closed

Native promises + node.js domains #555

ORESoftware opened this issue Mar 29, 2017 · 11 comments

Comments

@ORESoftware
Copy link

I like domains and use them for certain edge cases and libraries. Not for servers but for other things.

Has anyone in the Node.js community monkey-patched native promises to work with domains?

Is it possible? Looking for help with this. Thanks!

@ORESoftware
Copy link
Author

ORESoftware commented Mar 29, 2017

I wrote this up now:

nodejs/node-v0.x-archive#8648

this works:

const then = Promise.prototype.then;
Promise.prototype.then = function (fn1, fn2) {

  if (process.domain) {
    fn1 = fn1 && process.domain.bind(fn1);
    fn2 = fn2 && process.domain.bind(fn2);
  }

  return then.call(this, fn1, fn2);
};

process.on('uncaughtException', function(err){
    console.log(' This is uncaught => ', err);
});


const domain = require('domain');

const d = domain.create();

d.on('error', function(err){
  console.error(' => Domain caught => ',err);
});


d.run(function(){

  Promise.resolve().then(function(){
     process.nextTick(function(){
       throw new Error('rah');
     });

  });

});

be curious if there's more we should patch though

But seems to work

@shellberg
Copy link

I think its well recognised that Domains serve a developer's needs, but it created a maintenance nightmare in the core code (the tentacles of Domains penetrated everywhere!). Domains was an experiment; but it officially remains 'Deprecated' functionality. It persists, but with the hope that the usage exposure is only confined within the scope of well-maintained modules such that when its superceded the impact will be borne for the Node.js (end-)user community by only a few module maintainers. Just fair warning, because its sounds like your intention is to use Domains in your application code rather than in the module context.

As for promises... you're on safer ground with the standardisation of ES6 promises - you at least know the expectations for behaviour and specifications of what constitutes them given the many options of implementations hitherto.

You'll certainly find Domains has been a subject of much discussion : you could probably do worse than googling for some of that activity (some StackOverflow questions provide decent jumping off points ).
And, you should track progress on AsyncWrap which is a potential Domains successor and probably exactly what you want/need!

@ORESoftware
Copy link
Author

ORESoftware commented Mar 29, 2017

I have been following this thread of discussion for awhile. My concern is that AsyncWrap (or whatever it's called now - it changed names) does not solve the same problem that Domains solved.

If AsyncWrap can wrap an arbitrary amount of user code (or foreign code) and catch any and all errors that might get thrown, then - great - I don't need Domains anymore. However I don't believe that's the case - TMK AsyncWrap is only as good as what you do with it. In other words, it will only be guaranteed to work for the code you control.

I have heard the core maintenance headache argument - it's legit but I am starting to believe that it's not as bad as originally estimated.

Again, Domains become very useful, even imperative, not for application code, but for library code.

There seem to be two types of people in these arguments over domains - those that have used domains in their software and truly understand what they are about and those that haven't. The people maintaining core ultimately call the shots.

If you havent already I recommend you experiment with the code snippet above and you will see what domains can do.

@sam-github
Copy link

Domains couldn't be relied on either, not at first, they need cooperation from libs that did connection pooling.

Many libs slowly gained domain support, because domains were offiicial Node APIs. With time, asyncwrap support should also be adopted by the npm module ecosystem, but we aren't there yet, and I don't know if the APIs even exist (they were still under discussion at last NINA). Bluebird, for example, reasonably refused to add continuation-local-storage support, but when/if asyncwrap has APIs it can use, that might be more compelling.

I find it odd you say domains are imperative for library code, given the most common use was to wrap http request handlers, and libraries mostly just use domains when necessary to make sure callbacks occur in the correct domain, because error handling should be delegated to the user of a library. What do you find them so useful for in libraries?

@ORESoftware
Copy link
Author

ORESoftware commented Mar 29, 2017

domains are imperative for some library code. Here's some knowledge for you. Lab (for HapiJS) and Suman are test frameworks that both use domains to trap errors thrown from user code. This is a much better pattern that that used by Mocha (its a f*king nightmare over there). On the other hand, AVA partially solves the same problem in a different way - AVA has its own assertion library which will trap errors.

test(t => {
     t.assert(false);   // will throw, but the t object is attached to a test
});

AVA uses the above, and if t.assert throws, then AVA knows which test/hook is involved.

But, and a big but at that, if errors are thrown in AVA hooks that aren't thrown by its assertion library, than AVA is dead in the water. E.g.:

test(t => {
     t.assert(true);  // all good, no error thrown, but...
     process.nextTick(function(){
        throw new Error('imagine that this was thrown in some async code');
    });
});

That's why Suman and Lab can have a better dev experience than AVA and Mocha. AVA thought about using domains, the only reason they didn't is because they are deprecated, not because they wouldn't be a good solution, in fact some of the AVA maintainers were rueing the fact they were deprecated.

Software testing inverts the standard software model - errors are supposed to be thrown. Think about that for second. The library has to trap errors thrown from arbitrary code, we cannot expect the user to perfectly trap their own errors, the library is supposed to do much of that work.

To a certain extent testing software inverts the software development model - errors are supposed to be thrown, and aren't just for flow of control. They are supposed to be trapped and that's it. But how do you trap errors in arbitrary user code? Domains.

@ORESoftware
Copy link
Author

ORESoftware commented Mar 29, 2017

In other words, domains are the only solution to a small but significant subset of design problems in Node.js software. Testing libraries are one type of library that can benefit from consistent domain support.

By the way, Mocha solves the problem of "what test/hook through the error" by putting the test case in the global scope and attaching the error obtained in the uncaughtException handler to the globally defined test case. This is horrific and prevents test parallelization (tests have to run in series to support that pattern).

@ORESoftware
Copy link
Author

ORESoftware commented Mar 29, 2017

@sam-github but you are right, it's possible for users to write libraries that don't propagate domains. It requires buy-in from both core and userland. I will fully support AsyncWrap if it can do what domains can do, but at the moment, I do not have sufficient evidence to believe that.

@ORESoftware
Copy link
Author

ORESoftware commented Mar 29, 2017

@sam-github @shellberg here is your test:

take this code

const then = Promise.prototype.then;
Promise.prototype.then = function (fn1, fn2) {

  if (process.domain) {
    fn1 = fn1 && process.domain.bind(fn1);
    fn2 = fn2 && process.domain.bind(fn2);
  }

  return then.call(this, fn1, fn2);
};

process.on('uncaughtException', function(err){
    console.log(' This is uncaught => ', err);
});


const domain = require('domain');

const d = domain.create();

d.on('error', function(err){
  console.error(' => Domain caught => ',err);
});


d.run(function(){

  Promise.resolve().then(function(){
     process.nextTick(function(){
       throw new Error('rah');
     });

  });

});

replace domains with AsyncWrap and try to achieve the same functionality. You do this, and I will believe that AsyncWrap can do what domains do.

:)

that is all

@sam-github
Copy link

asyncwrap is a WIP, I don't claim it can replace domains yet, I don't think anyone does, but that's where the effort is going.

Mostly I was surprised by the "domains ... imperative for libraries". As for imperative for test runners, even my favorite test runner is trying to use domains now, I agree about that.

I've no comment on patching promises, it sounds like you got it working, maybe someone else will weigh in on what they've done in this space. @isaacs is using them in tap, for example.

@ORESoftware
Copy link
Author

ORESoftware commented Mar 29, 2017

@sam-github thanks for the feedback - so you are saying TapJS will use domains in the library?

@isaacs feel free to steal my promise+domains patch above. If something's wrong with it or incomplete with it, I'd like to know.

@ORESoftware
Copy link
Author

I see this issue - nodejs/node#10843
glad to see it

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

4 participants