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

domain migration to async_hooks #17143

Open
6 of 7 tasks
AndreasMadsen opened this issue Nov 19, 2017 · 37 comments
Open
6 of 7 tasks

domain migration to async_hooks #17143

AndreasMadsen opened this issue Nov 19, 2017 · 37 comments
Labels
async_hooks Issues and PRs related to the async hooks subsystem. domain Issues and PRs related to the domain subsystem.

Comments

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Nov 19, 2017

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.

/cc @addaleax @vdeturckheim

@AndreasMadsen AndreasMadsen added domain Issues and PRs related to the domain subsystem. async_hooks Issues and PRs related to the async hooks subsystem. labels Nov 19, 2017
@addaleax
Copy link
Member

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
Copy link
Member Author

AndreasMadsen commented Nov 20, 2017

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
Copy link
Member

vdeturckheim commented Nov 20, 2017

@AndreasMadsen
Copy link
Member Author

Added:

@addaleax
Copy link
Member

@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
Copy link
Member Author

@addaleax Yes, I think that would be fine.

addaleax added a commit to addaleax/node that referenced this issue Nov 26, 2017
Introduce `process.shouldAbortOnUncaughtException` to control
`--abort-on-uncaught-exception` behaviour, and implement
some of the domains functionality on top of it.

Refs: nodejs#17143
@vdeturckheim
Copy link
Member

@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
Copy link
Member Author

AndreasMadsen commented Nov 27, 2017

@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
Copy link
Member

lgtm, that's what I had in mind.

addaleax added a commit that referenced this issue Nov 29, 2017
Introduce `process.shouldAbortOnUncaughtException` to control
`--abort-on-uncaught-exception` behaviour, and implement
some of the domains functionality on top of it.

PR-URL: #17159
Refs: #17143
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
@AndreasMadsen
Copy link
Member Author

Added:

@vdeturckheim
Copy link
Member

@AndreasMadsen ccan you tick the event-related item?

@AndreasMadsen
Copy link
Member Author

@vdeturckheim Done 👍

@addaleax
Copy link
Member

addaleax commented Dec 10, 2017

@AndreasMadsen How would you feel about MakeCallback generating new async contexts based on the receiver object (e.g. via a WeakMap) when no async_context was passed?

That would allow us to remove .domain support from MakeCallback and might make sense as a transition stage anyway?

@TimothyGu
Copy link
Member

@addaleax Could a v8::Private be used?

@AndreasMadsen
Copy link
Member Author

How would you feel about MakeCallback generating new async contexts based on the receiver object (e.g. via a WeakMap) when no async_context was passed?

I don't see how you can emit the init event correctly or know that the content of async_context should be.

@addaleax
Copy link
Member

@TimothyGu I think so (although I was thinking NativeWeakMap)

@AndreasMadsen Yeah, init would have to be emitted before the call, coming from the void and reporting an unknown resource type. It’s not optimal. ;)

@AndreasMadsen
Copy link
Member Author

AndreasMadsen commented Dec 10, 2017

Yeah, init would have to be emitted before the call, coming from the void and reporting an unknown resource type. It’s not optimal. ;)

If that was the only concern it would be fine. What is important is that async_context have meaningful values that allow for propagation of context. How could we do this?

MylesBorins pushed a commit that referenced this issue Dec 12, 2017
Introduce `process.shouldAbortOnUncaughtException` to control
`--abort-on-uncaught-exception` behaviour, and implement
some of the domains functionality on top of it.

PR-URL: #17159
Refs: #17143
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
Introduce `process.shouldAbortOnUncaughtException` to control
`--abort-on-uncaught-exception` behaviour, and implement
some of the domains functionality on top of it.

PR-URL: #17159
Refs: #17143
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
@apapirovski
Copy link
Member

Just a note to say that I've updated the checklist in the OP with some recent changes & PR links.

@AndreasMadsen
Copy link
Member Author

Deprecate .domain support from MakeCallback (PR: #17417)

is now done :) We can "Remove .domain support from MakeCallback" once node.js 10.0.0 is released.

@AndreasMadsen
Copy link
Member Author

AndreasMadsen commented May 3, 2018

For moving forward with this, I would suggest:

node.js 11.0.0

  • we add a deprecation warning in node.js 11.0.0 for use of the entire domain module.
  • we move the current domain module to an external module, without the deprecation code.
  • I guess we will have to add an exception to the module resolution system, such it looks for an external module first when the user require('domain').

node.js 12.0.0

  • We remove the builtin domain module.
  • We revert the module resolution exception for require('domain').

The change to the module resolution is kinda nasty, but it is the only way I see that we could do the transition gracefully with a deprecation warning.

edit: The deprecation warning will just tell the user to run npm --save install domain. For old release that don't have the async_hooks support it won't be an issue, as they will resolve require('domain') to the builtin domain module.

@vdeturckheim
Copy link
Member

@AndreasMadsen LGTM.
At this point, domain is already taken on npm (https://www.npmjs.com/package/domain) and seems maintained (https://github.com/liangzeng/cqrs last commit yesterday).
We can either contact the maintainer or decide to go with something like @nodejs/domain.

@AndreasMadsen
Copy link
Member Author

I should add, some modules already integrate with domain. Properly they shouldn't. However, if we can get the domain npm module name, those modules should continue to work.

@vdeturckheim
Copy link
Member

As @apapirovski mentionned (#20474 (comment)), domain are still used in the REPL. Would we want to ship the external module in the REPL still?

@AndreasMadsen
Copy link
Member Author

@vdeturckheim Ah, I was not aware we used domain in the REPL. I don't really understand the purpose of the use, but we should migrate it to something else.

Even if we choose a different name for domain like @nodejs/domain. It will still be a problem as the deprecation warning will appear when using the REPL.

@vdeturckheim
Copy link
Member

@AndreasMadsen I took a first look and it does not look straightforward to remove. I'm not certain that replacing it will not lead to the creation of another domain-like API

@AndreasMadsen
Copy link
Member Author

@vdeturckheim I agree. It is quite a big concern. I'm not sure what the correct approach is here.

/cc @nodejs/repl

@addaleax
Copy link
Member

addaleax commented May 4, 2018

Can somebody explain why we want to remove the domain module? What’s the benefit, once we got it removed from our internal code?

We use it in the REPL to catch errors coming from user code. Since we expose the REPL in a programmatically accessible way (e.g. there can be multiple REPL instances per Node.js instance), and we need a way to distinguish errors created by each, I think removing the domain module would imply that we need to implement most of its functionality in the REPL module itself…

@vdeturckheim
Copy link
Member

@addaleax I have to admit I don't know the full context. I have always known this module in its deprecation phase and assumed this would lead to it being removed from Node. They have been discussions in a thread I opened this week (#20474) that made me think it was a goal to achieve.

TBH, I like this module and feel like it makes sense to keep it in Node. I am not sure what is the case against this module.

@AndreasMadsen
Copy link
Member Author

/cc @mcollina @AyushG3112 @ofrobots Care to comment on domain removal?

@mcollina
Copy link
Member

mcollina commented May 4, 2018

I think there are users that likes this module a lot. Our current position is that it was a mistake and it should not be used. If we want to add new features and improve this, then we should declare it supported and keep evolving it. Or we should move it to userland, and so the people that want this improved can work and improve it.

This PR was fine because it simplified things.

@ofrobots
Copy link
Contributor

ofrobots commented May 4, 2018

Domains have a few more tentacles into the code-base, e.g. process.domain. Those also need to be deprecated.

  • Domains were conceptually flawed - see the domains post-mortem. I do not believe they can be incrementally fixed. Significant changes in their semantics are needed to produce a proper domain- or zone-like API.
  • If the API is going to be substantially different, it would be better to call it something other than domains. I don't think we need to implement complicated module-resolution gymnastics in order to facilitate a movement of this module to from core to npm.
  • I'm +1 on starting to emit a runtime deprecation warning upon use of domains. The warning should encourage people to use alternatives from npm, e.g. zone.js, or whatever the userspace version of this module is going to be called etc.
  • domains in its current form can continue living in a frozen form in node core – I don't see urgency in removing it. We need to give ecosystem time to move away from it.
  • I would be strongly -1 on any feature additions or behaviour changes to the implementation of domains in core.

@AndreasMadsen
Copy link
Member Author

AndreasMadsen commented May 4, 2018

I'm +1 on starting to emit a runtime deprecation warning upon use of domains. The warning should encourage people to use alternatives from npm, e.g. zone.js, or whatever the userspace version of this module is going to be called etc.

Zones.js doesn't actually use async_hooks because of the current API limitations. I would like for there to be a working alternative to domain before deprecating it domain.

@ofrobots
Copy link
Contributor

ofrobots commented May 4, 2018

I would like for there to be a working alternative to domain before deprecating it.

SGTM. I just don't think we should do anything to domains that exists in core for now. An alternative can be built in user-space, and once it is there we can figure out the next steps.

@AyushG3112
Copy link
Contributor

AyushG3112 commented May 4, 2018

I also personally believe that domain should be left alone. True, it has its uses, but it has several implicit side effects(as mentioned in the port-mortem posted by @ofrobots, in #19998 etc) which are a pain to debug if you're not familiar with them.

However, the domain module is tightly coupled with some internals(like REPL) which make runtime deprecating it non-feasible for now, until we find a feasible replacement. I actually tried removing domain from REPL once, but it is going to take a lot of work.

@misterdjules
Copy link

I would like to encourage people to notify the @nodejs/domains, @nodejs/diagnostics and @nodejs/post-mortem teams when important discussions need to happen with regards to this PR.

I would also like to reiterate what others have mentioned in comments of this PR, which is that domains are used by a non-negligible number of users, and that unless keeping the domain module in core presents a significant burden to maintaining the core of Node.js, it should be left available without requiring code changes from consumers.

@Trott
Copy link
Member

Trott commented Apr 22, 2020

@AndreasMadsen Should this issue be left open or closed? I'm under the impression that there is no realistic intention of removing domain unless something external happens that forces Node.js to do so.

@AndreasMadsen
Copy link
Member Author

Looks like we still need to complete "Remove .domain support from MakeCallback". The depreciation code is still in master.

So I would suggest removing that code before closing this issue. I should make MakeCallback a lot simpler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. domain Issues and PRs related to the domain subsystem.
Projects
None yet
Development

No branches or pull requests

10 participants