Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix DisposeCapability leak #194

Merged
merged 7 commits into from Apr 10, 2024
Merged

Fix DisposeCapability leak #194

Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9faa60b
Add NOTE to DisposeResources to indicate disposeCapability can be freed
rbuckton Jul 19, 2023
d9ff42a
Add Asserts and set stack to empty on dispose
rbuckton Jul 19, 2023
4cb8de1
Minor tweak to NOTE text
rbuckton Jul 19, 2023
1796fa2
Use `undefined` instead of `~empty~`
rbuckton Mar 25, 2024
016d451
Switch to empty list
rbuckton Apr 10, 2024
372a40e
Minor cleanup
rbuckton Apr 10, 2024
f70e7e0
Update text of NOTE
rbuckton Apr 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion spec.emu
View file
Open in desktop
Expand Up @@ -927,7 +927,7 @@ contributors: Ron Buckton, Ecma International
[[DisposableResourceStack]]
</td>
<td>
A List of DisposableResource Records.
either *undefined* or a List of DisposableResource Records
</td>
<td>
The resources to be disposed. Resources are added in the order they are initialized, and are disposed in reverse order.
Expand Down Expand Up @@ -1018,6 +1018,7 @@ contributors: Ron Buckton, Ecma International
<dl class="header">
</dl>
<emu-alg>
1. Assert: _disposeCapability_.[[DisposableResourceStack]] is not *undefined*.
1. If _method_ is not present then,
1. If _V_ is either *null* or *undefined* and _hint_ is ~sync-dispose~, then
1. Return ~unused~.
Expand Down Expand Up @@ -1107,6 +1108,7 @@ contributors: Ron Buckton, Ecma International
</h1>
<dl class="header"></dl>
<emu-alg>
1. Assert: _disposeCapability_.[[DisposableResourceStack]] is not *undefined*.
1. For each _resource_ of _disposeCapability_.[[DisposableResourceStack]], in reverse list order, do
1. Let _result_ be Dispose(_resource_.[[ResourceValue]], _resource_.[[Hint]], _resource_.[[DisposeMethod]]).
1. If _result_.[[Type]] is ~throw~, then
Expand All @@ -1119,6 +1121,8 @@ contributors: Ron Buckton, Ecma International
1. Set _completion_ to ThrowCompletion(_error_).
1. Else,
1. Set _completion_ to _result_.
1. NOTE: After _disposeCapability_ has been disposed, it will never be used again. The contents of _disposeCapability_.[[DisposableResourceStack]] can be discarded at this point.
rbuckton marked this conversation as resolved.
Show resolved Hide resolved
1. Set _disposeCapability_.[[DisposableResourceStack]] to *undefined*.
Copy link

Choose a reason for hiding this comment

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

I'd set it to an empty list to avoid having to deal with a sentinel value everywhere else.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd set it to an empty list to avoid having to deal with a sentinel value everywhere else.

The upside of the sentinel value is that it makes things abundantly clear that a DisposeCapability is never intended to be reused, since we can assert on the absence of a list.

Copy link

Choose a reason for hiding this comment

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

You can also assert that the list is empty, no? In fact you can assert arbitrary English sentences, this isn't a program.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Asserting the list is empty would break DisposableStack/AsyncDisposableStack. Asserting a clearly defined state avoids ambiguity for spec readers and implementers. i.e.,

Assert: This DisposeCapability has never been passed as an argument to DisposeResources.

This seems way too ambiguous on its own.

Copy link

Choose a reason for hiding this comment

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

Asserting the list is empty would break DisposableStack/AsyncDisposableStack

I don't follow, say more?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DisposableStack and AsyncDisposableStack also use DisposeCapability, which starts with an empty list. This should not cause an Assert, or even a TypeError:

const stack = new DisposableStack();
stack.dispose(); // DisposeCapability.[[DisposableResourceStack]] will be empty here.

In addition, using x = null does not add an entry to DisposeCapability.[[DisposableResourceStack]] either (only await using x = null does to ensure we trigger an Await), so an empty list is a perfectly valid state for a non-disposed DisposeCapability.

Copy link

Choose a reason for hiding this comment

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

But you don't even need these asserts if you use an empty list. AFAICT the reasoning chain goes: you want to editorially convey that the list doesn't keep things alive -> you null it out with a sentinel value -> you add asserts that the sentinel values don't flow into places it shouldn't.

I still don't see what's lost by changing the above to: you want to editorially convey that the list doesn't keep things alive -> you empty the list. You get the same editorial effect, and fewer chances of editorial errors by flowing the sentinel into wrong places.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm concerned about the risk that a DisposeCapability is inadvertently reused. I am fine with changing this to just reset to an empty list, however.

Copy link
Member

Choose a reason for hiding this comment

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

If you do want to use a sentinel value, you. might want to use something more evocative, like ~disposed~.

1. Return _completion_.
</emu-alg>
</emu-clause>
Expand Down