-
-
Notifications
You must be signed in to change notification settings - Fork 34.2k
meta: add guidelines for introduction of ERM support #58526
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
Conversation
|
Review requested:
|
626622b to
51d96c1
Compare
Renegade334
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good on the whole, but it's worth pointing out that the role of disposability as stated here – basically a "sweeper" that avoids having to manually perform any last-ditch teardown (such as would occur in a finally block), but which still puts the onus on the user to explicitly invoke graceful disposal under normal execution – doesn't tally with the indicative cases from the tc39 proposal, which imply an environment wherein one doesn't need to explicitly close one's own disposable resources at all, and can just defer to the ERM disposer under both normal and abnormal conditions.
I don't have any strong feelings here, but probably warrants discussion.
|
@Renegade334 I think the way I would put it is that very few resources care about whether you are cleaning up because of a thrown exception or not. Those which do require special handling, but it's not something most APIs should need to think about. |
LiviaMedeiros
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I missed it, this document doesn't really cover creating handles like in #58453, i.e. these:
return {
[SymbolDispose]: () => this.removeListener(type, listener),
};Since we don't have using void yet and disposables are not always grouped by DisposableStacks, these objects usually will be exposed in userspace and have some user-defined names.
Should such objects be just this? Maybe they should be instances of some DisposableEntity class, implementing disposable interface? Or maybe they should be null-prototyped? Or maybe they should have Symbol.toPrimitive or kInspect that would help identifying what this object is? Or we should also add non-symbol dispose function (if so, we probably should come up with a generic name for all such objects)?
Personally I am happy with anonymous objects.
Don't see any reason for this - null prototypes make sense when you might have unknown keys (like
Definitely not
I do think there should be a string named dispose function. I don't think it should have a generic name - the disposal action is fundamentally different for different kinds of things (sometimes it's |
e77f4c7 to
17a85e3
Compare
17a85e3 to
e1f1ffb
Compare
|
Note: following the discussion at #58526 (comment), the filename still stands at |
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This document is very informative. Excited to see more of this implemented throughout core
54f2c30 to
bc957dc
Compare
|
I'll give this a couple more days for feedback. If there are no unresolved comments by end of the day Thursday, I will get this merged. |
bakkot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm but you might want to consider changing the setTimeout examples.
Renegade334
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few final copyedits and minor suggestions.
Looks good in general, certainly as a base to iterate on with experience. I do think that attempting graceful disposal in a could-have-errored context isn't necessarily as scary as these guidelines currently make out, but I imagine that more familiarity will lend a better sense of which paradigm is more useful.
LiviaMedeiros
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor stuff
LiviaMedeiros
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After rereading the whole thing, a few more suggestions below.
Additionally, I think this document should cover two more things:
- According to the description of the
DisposableInterface, ERM disposer should returnundefined, and async disposer should return a Promise that resolves withundefined. So, roughly speaking,
[SymbolDispose]() {
- return this.dispose();
- return this;
- return this.#statusCode;
- return true;
+ return void this.dispose();
+ this.dispose();
+ return;
+ // no return
}- To improve debugging experience,
[Symbol.dispose]function must not be a direct alias of named disposer function. This way, it would be possible to actually tell from stack traces if we're dealing withusingor conventional method.
- MyObject.prototype[SymbolDispose] = MyObject.prototype.dispose;
- MyObject.prototype[SymbolDispose] = MyObjectDisposeImpl;
- MyObject.prototype[SymbolDispose] = function dispose() { this.close(); }
+ MyObject.prototype[SymbolDispose] = assignFunctionName(SymbolDispose, function() {
+ this.idempotentDispose();
+ });c6b4cac to
960b1ff
Compare
LiviaMedeiros
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with suppressing the linter
35bb55d to
4f75bef
Compare
|
Landed in ebe7dad |
PR-URL: #58526 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]>
Stemming from discussions in #58516 , this seeks to add guidance for introducing explicit resource management support into existing Node.js APIs. This is mean to be discussed and evolved so please weigh in.
/cc @nodejs/tsc @nodejs/collaborators @bakkot