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.

[legendecas]

I'd disagree with recommending returning an anonymous objects in an API design guideline. We should recommend structured API to improve comprehensiveness and discoverablity.

[bakkot]

@legendecas The problem is that lots of APIs need a bespoke disposable which isn't used for anything else, and adding a new class for every such API ends up littering lots of little classes all over the place. See e.g. #58516 for such an API. I think anonymous objects are better than having a separate class for each such API.

(But if you disagree, leave a comment on that PR as well.)

[jasnell]

This is likely something that can be best handled in documentation than code. For instance, in code we can have it be a fully anonymous object:

return {
  dispose() { ... },
  [Symbol.dispose]() { this.dispose(); }
}

While in documentation is can at least appear to be a named interface:

### `foo()`

* Returns {Disposable}

### `Disposable`

#### `disposable.dispose()`

#### `disposable[Symbol.dispose]`

So I'd suggest that we're talking about a documentation difference here, not necessarily a coding difference.

[legendecas]

I think the question is more like how user anticipates with a disposable API. If an API returns an object, it should be easy to know what users can do with the object.

#58516 is an example that there are more properties than the documented path on the returned anonymous object, like remove(). It is not ideal to just say "the returned object has property A, B, and C". We should document every public API and its behavior clearly if possible.

Regarding "adding a new class for every such API ends up littering lots of little classes all over the place", IMO this is a question on if it is worthwhile to add a new API to add disposable support. If an API already returns an object, there is no need to add a new disposable class for it.

[mcollina]

LGTM

Can you add an example about asyncDispose as well?

[Ethan-Arrowood]

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

[Renegade334]

This came up in an undici PR, and is also probably worth stating – again, wording flexible:

Suggested change

	7. Avoid, as much as possible, using both `Symbol.dispose` and `Symbl.asyncDispose`
	7. Because the disposal process is strictly ordered, there is an intrinsic
	expectation that all tasks performed by a single disposer are fully complete
	at the point that the disposer returns. This means, for example, that
	"callback-style" APIs must not be invoked within a disposer, unless they are
	promisified and awaited. Any Promise created within a disposer must be
	awaited, to ensure its resolution prior to the disposer returning.
	8. Avoid, as much as possible, using both `Symbol.dispose` and `Symbol.asyncDispose`