This repository has been archived by the owner on Jan 25, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 40
Editorial: Document "independent lifetime" of FinalizationGroups #184
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
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
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.
It might be worth explicitly mentioning that if the only remaining tasks an implementation has are finalization callbacks, it can probably consider that as the time to exit.
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 intention was that the first bullet point would cover this, but maybe it's too unclear. If you have a suggestion for another wording of the paragraph, or wording for an additional bullet point, that's more clear, I'd appreciate that. I'm not sure if we have a clear cross-environment meaning for "time to exit", so any wording would probably be best if it can somehow acknowledge that.
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.
@littledan I think a lot of people have this idea of JS's loop where you throw a bunch of items in and JS keeps chugging along until you either explicitly stop it (what you mention in bullet two), or it runs out of items to process. Your PR to 262 and this proposal make handling the loop a lot more abstract, but I think it is still worth explicitly mentioning because it's kind of a shift in the semantics of how you'd implement 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.
Please, suggest some wording here. My intention has been to speak specifically to that, and I'm still having trouble understanding what the following paragraph is missing (since it specifically talks about callbacks not being called on shutdown).
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.
"If the only remaining tasks are finalization tasks, the implementation should consider the queue empty, otherwise it may be forced into an infinite loop of finalization"
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.
IMO, an implementation could do something like: "has the program consumed at least one element from the iterator? Then I may give it another chance to consume more. Else, thank you and goodbye".
Not saying implementations should do this, but they could.
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.
@mhofman the case here is that the implementation isn't planning to exit, it's just running to the completion of the queue. Like if you make a node script that creates a file, you don't have to explicitly do process.exit() at the end, it just exits when there's nothing left in the queue.
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.
What does "the queue" refer to even? (We're not requiring engines to have a "job queue", just to schedule the finalization at some point maybe.) I don't understand how this statement should be read by JS developers, who will be the vast majority of people who read this document. The infinite loop of finalization sounds like it makes sense in the context of your JS implementation, but I don't think it makes sense across environments, or without context, so I don't really think it makes sense to refer to it that way in this document.
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.
@littledan "the queue" being the abstract place the implementation keeps track of "scheduled" finalization callbacks. I assume this doesn't affect other engines because they already do what I'm suggesting mentioning here, which is why I think it should be mentioned.
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.
Sorry, I think I'll omit this paragraph; it just seems like a more confusing version of what I have written below. From a user perspective, the callbacks are skipped on the way out; to implement that, yes, you exit before exhausing the queue, but this is really the same thing.