Enable FinalizationRegistry #3560
+121
−12
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1431,7 +1431,7 @@ class ResourceWrapper { | |
| // We do not allow use of WeakRef or FinalizationRegistry because they introduce | ||
| // non-deterministic behavior. | ||
| check(global->Delete(context, v8StrIntern(isolate, "WeakRef"_kj))); | ||
| //check(global->Delete(context, v8StrIntern(isolate, "FinalizationRegistry"_kj))); | ||
|
There was a problem hiding this comment. This probably needs a compat flag as I suspect there are people that check for the existence of these APIs and use them if they are available -- making them suddenly available would therefore cause such workers to start taking a new code path which could end up being broken. There was a problem hiding this comment. Yeah that was something on my mind as well, had a brief discussion with @mikenomitch about placing this behind a compat date/flag. There was a problem hiding this comment. This makes me sad, but yes, compat flag absolutely needed. I've seen such checks in the wild a few times now. |
||
|
|
||
| // Store a pointer to this object in slot 1, to be extracted in callbacks. | ||
| context->SetAlignedPointerInEmbedderData(1, ptr.get()); | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd be enabling
WeakReftoo right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current plan is to only enable
FinalizationRegistry, given the immediate memory cleanup benefits for wasm users.WeakRefis something we can probably think about as a followup.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there actually anything more needed to enable WeakRef? If it's just a matter of removing the line above, I think we should do it in this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it should work by just removing the line, although @harrishancock was concerned about
WeakRefgiving immediate notification of GC collection instead ofFinalizationRegistrywhich is more non-deterministic.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WeakRefdoes not give immediate notification thankfully. I suppose someone could poll it as quickly as possible to approximate immediate notification but thankfully it does not provide a notification api.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern isn't immediate notification, but lack of control over when notifications occur. With FinalizationRegistry, we can control exactly when finalization callbacks are scheduled. With WeakRef, we have no control over when they appear empty -- that's entirely up to the GC. So, if controlling the timing of GC observation is important for risk mitigation, then enabling WeakRef is strictly higher risk than enabling FinalizationRegistry.
That said, I'd love to just accept the risk and enable WeakRef and be done with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with accepting the risk as I thinks it is likely quite minimal. @kentonv ?