-
Notifications
You must be signed in to change notification settings - Fork 31
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
[Symbol.enter]()
Follow-on proposal
#195
Comments
I'm not a huge fan of the name
The thing is essentially all host APIs also auto cleanup on GC of the object (even when flaky). For userland though it's actually fairly annoying to add this behaviour as it's both tends to result in a lot of extra boilerplate and it's way too easy to hold a reference to the wrapper object. Honestly the feature would be a lot less useful if this were possible to write: @autoDisposeOnGC
class SomeDisposable {
[Symbol.dispose]() {
this.#diposeSomehow();
}
} but at present it isn't possible as I do wonder though if const disposalOrphanage = new Orphanage(semiWeakRef => {
const disposable = semiWeakRef.deref();
semiWeakRef.release();
disposalOrphanage.unregister(disposable);
disposable[Symbol.dispose]();
});
function autoDisposeOnOrphaned(klass, ctx) {
return class extends klass {
constructor(...args) {
super(...args);
const semiWeakRef = new SemiWeakRef(this);
disposalOrphanage.register(this, semiWeakRef, this);
}
}
}
@autoDisposeOnOrphaned
class SomeDisposable {
[Symbol.dispose]() {
this.#disposeSomehow();
}
} |
Interesting, I hadn't thought of using I do still intend to propose a mechanism along the lines of the That said I'm skeptical of using it as a simple pre-finalization "disposer", especially when a simple refactor of the disposable can effectively do the same. I actually think you might be able to achieve almost the same auto-dispose decorator convenience with a |
I'm not much of a fan of
A base class is probably more feasible, since you can pass down a resource handle and cleanup method to a superclass constructor, but it could still be achievable with decorators, i.e. class SomeDisposable {
@CleanupOnGC(handle => doCleanup(handle))
#handle;
@PreventCleanupOnGC()
[Symbol.dispose]() {
doCleanup(this.#handle);
}
} or even without decorators: class SomeDisposable {
#handle;
constructor() {
...
this.#handle = new SafeHandle(handle, handle => doCleanup(handle));
}
[Symbol.dispose]() {
this.#handle.dispose();
}
} |
Maybe |
Yeah I've been wanting it for a while for basically the same purpose, so it's been on my mind. Thinking about it more though (Well it could work if there were some kind've ordered-orphanage where ordered-orphanages created earlier only observe that an object is orphaned if all ordered-semiweakrefs created after the ordered-orphanage are also released. I think engines adding this unlikely.)
To an extent, but I don't think there's any way to do so without changing some other observable behaviour. e.g. In the example: @autoDiscardOnGC
class SomeDisposable {
method(someValue) {
// (1)
this.#whatever();
// (2)
someValue.method(this);
// (3)
if (someValue === someSpecificValue) {
}
}
}
const d = new SomeDisposable();
d.someMethod(someSpecificValue); regardless of the implementation of autoDiscardOnGC one of the following must be true within the method:
in all cases observable differences might happen (beyond the desired auto-dispose-on-GC) which makes the thing honestly more risky to use than just dealing with finalization themselves. |
I'd be in favour of a |
In #203, there's the question of detecting whether a disposable resource was correctly instantiated via In this discussion, we're talking about the relationship between the disposable resource and the object received by the caller of Has any thought been given to the idea of a resource instead producing a disposable? For example, let's imagine a couple new protocols class File {
static #FileDisposer = class {
#file;
constructor(file) {
#file = file;
}
[Symbol.dispose]() {
if (this.#file.#fd) {
const fd = this.#file.#fd;
this.#file.fd = undefined;
closeSync(fd);
}
}
}
#fd;
constructor(filename) {
this.#fd = openSync(filename);
}
[Symbol.disposer]() {
if (this.#fd) return new File.#FileDisposer(this);
throw new TypeError();
}
read(...) { ... }
write(...) { ... }
} In this way a resource can detect that it has been bound using the I think an interesting side-benefit of a model like this is that the author of a resource can opt into building their own reference counting since they could observe the number of different referenced handles through calls to |
I would prefer |
@yw662 since marking an item as disposable can be done with DisposableStack and not just |
I've been considering this for some time, and I'm still not convinced a In addition, there exists two remediations for the lack of
I do believe that |
Does any browser actually implement I disagree that it's niche use, though. The lack of @rbuckton To be clear, by your second remediation, you mean to use |
V8 is currently working on an implementation, and
The majority of APIs that are concerned with native handles are usually exposed by the host and often already have
I strongly disagree with this sentiment. As-is, To be clear, I'm not saying that
I mean something like the following: class SomeResource {
#nativeHandle;
#disposeState = "unregistered";
...
// user code that would use the handle if the resource is in a valid state
doSomethingWithResource() {
this.#throwIfInvalid();
...
}
// treat reading the Symbol.dispose method as registration
get [Symbol.dispose]() {
if (this.#disposeState === "unregistered") {
this.#disposeState = "registered";
}
return SomeResource.#dispose;
}
// actual `Symbol.dispose` method implementation
static #dispose = function() {
if (this.#disposeState === "disposed") {
return;
}
const nativeHandle = this.#nativeHandle;
this.#disposeState = "disposed";
this.#nativeHandle = undefined;
cleanupNativeHandle(nativeHandle);
};
// verify resource is in the correct state
#throwIfInvalid() {
switch (this.#disposeState) {
case "unregistered":
throw new TypeError("This resource must either be registered via 'using' or attached to a 'DisposableStack'");
case "disposed":
throw new ReferenceError("Object is disposed");
}
}
} In general it's a best practice to verify the state of a wrapped resource before acting on it, and while that would usually mean just checking whether the resource is disposed, it can be expanded as I have here to check whether the resource is registered.
This is feasible to do in a linter, to an extent. Receiving a Disposable as a return value (except from, say, a |
There's always an option to have a new set of APIs instead of upgrading existing ones. Even if you upgrade existing APIs, none of the existing code using them gets any kind of benefits you introduced. It's only new code or refactored code can benefit from your upgrade. If that's the case, new code may be better off using new set of APIs that are both easier to dispose and safer to use, meaning enforcing
I strongly disagree. You overlooked the importance of making a safe disposal mechanism simple and accessible. There are so many common use cases that doesn't involve native resources and still need to clean up or there can be resource leak. For example event listeners, HTTP requests, long running Promises just to name a few.
const stack = new DisposableStack();
try {
// lines before using x
const x = stack.use(resource());
// lines after using x
const y = stack.use(resource());
// lines after using y
} finally {
stack.dispose();
} Tell me what are the benefits
I think we only have one chance to get it right. It's now or never.
If there's a working linter that can catch most of the |
It is a simple matter to refactor existing code to
That is quite literally the point.
On its own, it's not much safer than existing mechanisms. It's too easy to incorrectly call const stack1 = new DisposableStack();
try {
const res1 = stack1.use(getResource1());
const stack2 = new DisposableStack();
try {
const res2 = stack1.use(getResource2()); // oops, wrong stack
}
finally {
stack2.dispose();
}
// continue to use res1
}
finally {
stack1.dispose();
} However, when composing disposables, class C {
#disposables;
#res1;
#res2;
constructor(options) {
using stack = new DisposableStack();
this.#res1 = stack.use(getResource1(options));
this.#res2 = stack.use(getResource2(options)); // if this throws, need to clean up
this.#disposables = stack.move(); // ok, everything succeeded, track things we need to clean up later
}
[Symbol.dispose]() {
this.#disposables.dispose();
}
} This is significantly harder to get right without
I disagree. Implementations can slot in
I'll look into adding a rule for |
It's 2024 already. There are many successful resource management prior arts out there for us to learn from. I don't understand why we have to stick with the C# flavor out of all alternatives. Developers nowadays deserves safer guarantees when we design new language features. Even Joe Biden understand the importance of accessible memory/resource safety guarantees. I'm just not convinced that having a seamless upgrade for existing APIs can be more important than making the language feature to be safer.
I won't call that a mistake at all. Whether Writing const getResource1 = () => { [Symbol.dispose]() {} };
const getResource2 = () => { [Symbol.dispose]() { throw new Error(); } };
{
using res1 = getResource1();
using res2 = getResource2();
console.log("done"); // this will print
}
{
using res1 = getResource1();
{
using res2 = getResource2();
}
console.log("done"); // this won't print
} Just imagine how much mental burden you are imposing on code reviewers by adopting the
There are so many use cases other than just class constructors where we want to extend the lifetime of resource beyond the enclosing lexical block.
A seamless upgrade for this feature is really not necessary, and probably can cause more confusion for developers, because there would be two difference ways of calling the same API. FWIW, we had experienced "bifurcate" changes when we migrated from callback based APIs to Promise based APIs. Although it's not the smoothest transition in the world, but nowadays people can pick it up quite easily without any problem. And fortunate for us, unlike the migration from callbacks to Promises, there are not that many APIs that manages resource. It won't be as overwhelming as the Promise transition. What if we just keep it a TypeScript feature instead of pushing it to JavaScript? Or just move forward without the |
To clarify what I meant by:
Imagine an API got upgraded, how can a linter help users to adopt it? // From some library that upgraded their API:
interface Resource extends Disposable {
close(): void;
}
function createResource(): Resource;
// In some user's code:
{
const res = createResource();
// ^-- eslint: Consider change it to `using res = createResource();`
try {
// ...
} finally {
res.close();
}
} Now, how likely the user would just make the easiest change and do: {
using res = createResource();
try {
// ...
} finally {
res.close();
}
} Now the resource would be closed twice. Which may or may not cause problems but it's generally not a good idea, and often will cause issues. |
I did not propose this as a TypeScript feature that we want in JavaScript. I proposed this as a JavaScript feature that supports other JavaScript proposals, such as the Shared Structs proposal which needs a convenient mechanism for handling Mutex lock lifetime.
A more robust description of the lint failure can go a long way to informing users how to correctly address the issue. If the API is a recognized host API, the linter can be even more specific and recognize such cases. A well defined Disposable should ideally handle double-dispose correctly, with
This is already a blend of multiple similar capabilities across several languages, though it is heavily influenced by both C# and Python as prior art. |
I have added https://github.com/rbuckton/proposal-using-enforcement to the agenda for the upcoming TC39 plenary. |
This suggestion 💯. I've found it difficult to adopt
// existing code
function client() {
return {
// ...
destroy() {},
}
}
// using-compatible utility
function disposableClient() {
return { value: client(), [Symbol.dispose]() { client.destroy(); } };
}
// usage
{
using clientWrapper = disposableClient();
const client = clientWrapper.value;
}
{
using myResource = resource();
myResource[Symbol.dispose]();
myResource.doThing(); // oopsie
} When using Side note: Obviously |
There has been some interest in changing the behavior of
using
andawait using
slightly to guide users towardsusing
overconst
. In #49 there is a more comprehensive discussion about the possibility of implementing full Python-like Context Manager semantics, but an alternative to this would be the addition of a very lightweight[Symbol.enter]()
method to the Disposable protocol.If adopted, a
[Symbol.enter]()
method would be used to produce the actual resource you intend for a user to interact with, i.e.:For a statement like
the resulting
File
object would be tracked along with its[Symbol.dispose]()
method in the environment, but the result of its[Symbol.enter]()
method would be what is actually stored inx
.This would guide API consumers towards
using
since the only way to interact with the resource viaconst
would be to explicitly call[Symbol.enter]()
, which provides a way to opt-out ofusing
should you need it.However, most existing APIs provided by hosts are not designed for such a mechanism, and I imagine there is little appetite amongst implementers to draft an entirely parallel set of APIs to support this construct. Existing APIs could be adapted by implementing a
[Symbol.enter]() { return this; }
, which would result in behavior that is comparable to the current proposal. In fact, this is the default behavior of Python Context Managers as well.Given that the default behavior would be
[Symbol.enter]() { return this; }
, we are considering[Symbol.enter]
itself to be a purely optional mechanism that an API producer could implement to opt-in to more pedantic semantics aroundusing
andawait using
, without burdening all future disposables with such an API, much like how anIterator
today only mandates the presence of anext()
method, withreturn()
andthrow()
being purely optional.Given that we intend for
[Symbol.enter]()
to be optional, we are hoping to pursue its addition as a follow-on proposal that we could potentially add after Stage 4, giving us time to see how the ecosystem responds tousing
andawait using
and whether the addition of[Symbol.enter]()
is strictly necessary.Please note that we are not currently considering a
[Symbol.asyncEnter]()
method as part of this discussion. If the stated goal is to "guide users towardsusing
andawait using
", then the presence of[Symbol.enter]
and[Symbol.asyncDispose]
are sufficient indicators of intent. A Promise-returning[Symbol.asyncEnter]()
method would introduce far too much complexity and introduce additional implicitawait
s into your code, which we do not think is merited given the narrow scope of this change.Related discussions
The text was updated successfully, but these errors were encountered: