meta: add guidelines for introduction of ERM support

[jasnell]

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

[nodejs-github-bot]

Review requested:

• @nodejs/tsc

[Renegade334]

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]

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

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]

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.

[Renegade334]

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

[Ethan-Arrowood]

This document is very informative. Excited to see more of this implemented throughout core

[jasnell]

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]

lgtm but you might want to consider changing the setTimeout examples.

[Renegade334]

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.

[Renegade334]

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

[Renegade334]

Do the content paragraphs in this section need indenting?

[LiviaMedeiros]

Minor stuff

[LiviaMedeiros]

After rereading the whole thing, a few more suggestions below.

Additionally, I think this document should cover two more things:

1. According to the description of the Disposable Interface, ERM disposer should return undefined, and async disposer should return a Promise that resolves with undefined. So, roughly speaking,

[SymbolDispose]() {
-  return this.dispose();
-  return this;
-  return this.#statusCode;
-  return true;
+  return void this.dispose();
+  this.dispose();
+  return;
+  // no return
}

2. 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 with using or 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();
+ });

[LiviaMedeiros]

I wonder if it something like "This is similar to how finally { } block works" should be added here. Thinking of disposers as of implicit finally and projecting usual patterns might help folks with understanding what should and should not be done in disposer much faster than trying to learn all these quirks as completely novel feature.

[LiviaMedeiros]

Being pedantic here...

1. The "explicit disposal methods" sounds incorrect in this context. Maybe "named disposal methods", or "conventional disposal methods"?

2. Nothing stops the userland code from calling [Symbol.dispose]() directly, so technically implementing an ERM disposer is enough, and adding a string-named method sounds like an unnecessary code duplication.

Also this slightly contradicts with the statement from above:

Importantly, it is necessary to understand that the design of using makes it possible for user code to call the Symbol.dispose or Symbol.asyncDispose methods directly, outside of the using or await using statements.

---

Thus said, I agree with this recommendation but I think this whole part should be reworded. It must be clear that this is simply a recommended best practice, and it should provide the rationale behind it.

The issue is that if userland is forced to call ERM disposers directly, it would hurt readability and obscure debugging in case of error: people should always see [Symbol.dispose]() in their stack trace if using was used, and conventional method otherwise.

Therefore, it's Node.js's responsibility to provide conventional method, and maybe even highlight in the API docs that for direct calls it's preferable over ERM disposers.

[LiviaMedeiros]

Should these be links?

Suggested change

	for instance, in the documentation of the Web Crypto API in
	`/doc/api/webcrypto.md`.
	for instance, in the documentation of the [Web Crypto API](https://nodejs.org/api/webcrypto.html) in
	[`/doc/api/webcrypto.md`](https://github.com/nodejs/node/blob/HEAD/doc/api/webcrypto.md).

[LiviaMedeiros]

I think it would be convenient to have link to the proposal (or to MDN once there are comprehensive docs for this).

Suggested change

	Explicit Resource Management is a capability that was introduced to the JavaScript
	[Explicit Resource Management](https://github.com/tc39/proposal-explicit-resource-management) is a capability that was introduced to the JavaScript