domain migration to async_hooks

[AndreasMadsen]

The PR #16222, contains the first step by @vdeturckheim to remove domain from the deep layers of nodecore and implement it on top of async_hooks. This PR just landed, thus I'm opening this issue to discuss the next steps.

• Deprecate and remove .domain support from MakeCallback. Implementers should use async_context. What will our depreciation and removal strategy for this be? Note: we would like to ultimately remove this as it adds quite a bit of extra complexity.
  • Deprecate .domain support from MakeCallback (PR: domain: runtime deprecate MakeCallback #17417)
  • Remove .domain support from MakeCallback
• Remove special domain code in exception handling. It is unclear what the migration strategy for this is. (PR: process: add capture function for uncaught exceptions #17159)
• Remove domain integration in events. (PR: events: remove domain handling from events to domain #17403, PR: domain: fix error emit handling #17588)
• Check for leftovers. Examples:
  • timers (PR: timers: remove domain enter and exit #17880)
  • env declarations (PR: domain, src: clean up domain-related code #18291)
• Track context with async_hooks (PR: domain: re-implement domain over async_hook #16222)

/cc @addaleax @vdeturckheim

[addaleax]

Remove .domain support from MakeCallback. Implementers should use async_context. What will our migration strategy for this be?

I’d say we should runtime deprecate support for that once async_hooks is stable?

Remove special domain code in exception handling. It is unclear what the migration strategy for this is. Do we need extra API in nodecore?

Yes. We need:

• A way to control the return value of ShouldAbortOnUncaughtException() without domains. I think I’ve talked about this a bit with @apapirovski in the context of [WIP] events: use Map for events and limit copying #17074; basically, my idea would be to expose something like process.setShouldAbortOnUncaughtException(bool) that controls a 1/0 boolean flag, and that would be set from the domains code (but also available to userland that doesn’t use domains). An additional niceness would be getting rid of the calls into the VM inside that C++ code, technically speaking none of that is allowed.
• A way to emulate the uncaught exception behaviour from process._fatalException – we’d either need to expose a second API in addition to uncaughtException, or we might say that we’ll come close enough with letting domains use prependListener('uncaughtException').

[AndreasMadsen]

I’d say we should runtime deprecate support for that once async_hooks is stable?

Honestly, I would go one step further and runtime deprecate it now and remove it once async_hooks is stable. Getting async_hooks stable will likely take some time and this could provide valuable feedback for us. I doubt this API is used by many and it is not like async_hooks is more experimental than domain already is, especially now that domain actually uses async_hooks.

[vdeturckheim]

@AndreasMadsen I think async_hook is now in production in a large number of apps since New Relic uses it without flag in their agents:

https://blog.newrelic.com/2017/11/08/node-8-async-await-support/

https://github.com/newrelic/node-newrelic/blob/5f988394649fd3d1b8fd2e30a3c8a38ac2ed49e5/lib/instrumentation/core/async_hooks.js#L54

[AndreasMadsen]

Added:

• Remove domain integration in events.

[addaleax]

@AndreasMadsen Any ideas for that? I’d probably just replace EventEmitter.prototype.emit – that should be okay given that it’s a public API that’s never going to change?

[AndreasMadsen]

@addaleax Yes, I think that would be fine.

[vdeturckheim]

@AndreasMadsen if I understand well, I just need to move the domain related code from events.js to domain.js? If so, I should be able to open a PR this week.

[AndreasMadsen]

@vdeturckheim That would be awesome. It should be possible to do it with a monkey-patch of EventEmitter or similar hack. More creative solutions is of course appreciated as well.

[vdeturckheim]

lgtm, that's what I had in mind.