Open AndreasMadsen opened 6 years ago
Remove
.domain
support fromMakeCallback
. Implementers should useasync_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:
ShouldAbortOnUncaughtException()
without domains. I think I’ve talked about this a bit with @apapirovski in the context of https://github.com/nodejs/node/pull/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.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')
.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
.
@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/
@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?
@addaleax Yes, I think that would be fine.
@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.
@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.
lgtm, that's what I had in mind.
Added:
@AndreasMadsen ccan you tick the event-related item?
@vdeturckheim Done 👍
@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?
@addaleax Could a v8::Private
be used?
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.
@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. ;)
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?
Just a note to say that I've updated the checklist in the OP with some recent changes & PR links.
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.
For moving forward with this, I would suggest:
node.js 11.0.0
require('domain')
.node.js 12.0.0
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.
@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
.
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.
As @apapirovski mentionned (https://github.com/nodejs/node/issues/20474#issuecomment-386502549), domain are still used in the REPL. Would we want to ship the external module in the REPL still?
@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.
@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
@vdeturckheim I agree. It is quite a big concern. I'm not sure what the correct approach is here.
/cc @nodejs/repl
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…
@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 (https://github.com/nodejs/node/issues/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.
/cc @mcollina @AyushG3112 @ofrobots Care to comment on domain removal?
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.
Domains have a few more tentacles into the code-base, e.g. process.domain
. Those also need to be deprecated.
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.
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.
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.
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.
@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.
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.
The PR https://github.com/nodejs/node/pull/16222, contains the first step by @vdeturckheim to remove
domain
from the deep layers of nodecore and implement it on top ofasync_hooks
. This PR just landed, thus I'm opening this issue to discuss the next steps..domain
support fromMakeCallback
. Implementers should useasync_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..domain
support fromMakeCallback
(PR: https://github.com/nodejs/node/pull/17417).domain
support fromMakeCallback
It is unclear what the migration strategy for this is.(PR: https://github.com/nodejs/node/pull/17159)async_hooks
(PR: https://github.com/nodejs/node/pull/16222)/cc @addaleax @vdeturckheim