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

discussion/question - api implementation makes sense just not quite expected #22

Open
ORESoftware opened this issue Nov 2, 2023 · 1 comment

Comments

@ORESoftware
Copy link

ORESoftware commented Nov 2, 2023

I enjoyed the old domain API, something like this:

const Domain = require('domain');
const d = Domain.create();

d.run(() => {
    console.log('running');
});

d.once('error', e => {
    console.error('domain caught:',e);
});

throw 'boop'

boop would not get captured by the domain, instead it bubbles up to global space

however, when using AHD - we have:

const ahd = require('async-hook-domain');

const d = new ahd.Domain(err => {
    console.error('AHD caught:',err);
});

throw 'boop'

and boop will get captured by AHD.

So this begs the question, does the original domain module process.nextTick() when it executes the run method? I looked through the source back in the day and didn't notice that - but I suspect if so, that could be one of the biggest sources of slower performance?! And again if so, it would be very hard to change the old core domain API to not process.nextTick...so maybe that's why original domain is stuck deprecated. just a theory.

@ORESoftware
Copy link
Author

ORESoftware commented Nov 2, 2023

nevermind, it doesn't process.nextTick, it does this:

Domain.prototype.run = function(fn) {
  this.enter();
  const ret = ReflectApply(fn, this, ArrayPrototypeSlice(arguments, 1));
  this.exit();  // here
  return ret;
};

so then I can't explain to myself why the original implementation should be slow. So I am wondering what the culprit is/was. Current implementation is partly based off async-hooks - https://github.com/nodejs/node/blob/main/lib/domain.js#L74C31-L74C31

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

1 participant