Skip to content

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented May 31, 2025

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

@jasnell jasnell added the meta Issues and PRs related to the general management of the project. label May 31, 2025
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label May 31, 2025
Copy link
Contributor

@Renegade334 Renegade334 left a 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.

@bakkot
Copy link
Contributor

bakkot commented May 31, 2025

@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.

Copy link
Member

@LiviaMedeiros LiviaMedeiros left a 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)?

@bakkot
Copy link
Contributor

bakkot commented May 31, 2025

Should such objects be just this? Maybe they should be instances of some DisposableEntity class, implementing disposable interface?

Personally I am happy with anonymous objects.

Or maybe they should be null-prototyped?

Don't see any reason for this - null prototypes make sense when you might have unknown keys (like groupBy or parseArgs), or more rarely when it's important that users be able to rely on absence of properties which might be inherited from Object.prototype in the presence of malicious code, but neither is the case here.

Or maybe they should have Symbol.toPrimitive or kInspect that would help identifying what this object is?

Definitely not toPrimitive; almost nothing should have toPrimitive except like Date. They could have a toStringTag or a kInspect, maybe, but only if you'd do that with every other kind of object.

Or we should also add non-symbol dispose function (if so, we probably should come up with a generic name for all such objects)?

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 close, sometimes it's release, sometimes it's remove, whatever). The name should be chosen to be informative to the reader.

@jasnell jasnell force-pushed the jasnell/erm-guidelines branch 2 times, most recently from e77f4c7 to 17a85e3 Compare June 5, 2025 02:29
@jasnell jasnell force-pushed the jasnell/erm-guidelines branch from 17a85e3 to e1f1ffb Compare June 5, 2025 13:36
@Renegade334
Copy link
Contributor

Note: following the discussion at #58526 (comment), the filename still stands at erm-guidelines.md, which may hinder accessibility.

Copy link
Contributor

@Ethan-Arrowood Ethan-Arrowood left a 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

@jasnell jasnell force-pushed the jasnell/erm-guidelines branch from 54f2c30 to bc957dc Compare June 24, 2025 03:54
@jasnell
Copy link
Member Author

jasnell commented Jun 24, 2025

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.

Copy link
Contributor

@bakkot bakkot left a 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.

Copy link
Contributor

@Renegade334 Renegade334 left a 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.

}
```

### A Note on documenting disposable objects
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Headings could do with having uniform casing -- there's a mixture of Sentence case, Headline Case, and something in-between.

Comment on lines +375 to +385
```js
function foo(someObject) {
using resource = someObject;
}
```

The reason this is problematic is that the `using` statement will
unconditionally call the `Symbol.dispose` method on `someObject` when the block
exits, but you do not control the lifecycle of `someObject`. If `someObject`
is disposed of, it may lead to unexpected behavior in the rest of the
code that called the `foo` function.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the content paragraphs in this section need indenting?

Copy link
Member

@LiviaMedeiros LiviaMedeiros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor stuff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.