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.

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

Editorial: Clarify that errors are not swallowed #76

Merged
merged 2 commits into from Apr 6, 2019

Conversation

Projects
None yet
4 participants
@littledan
Copy link
Member

littledan commented Apr 5, 2019

The specification already contains semantics here, but they are a bit obscure, so this patch adds a note that clarifies what happens. The issue was raised at https://bugs.chromium.org/p/v8/issues/detail?id=8179#c30

@littledan

This comment has been minimized.

Copy link
Member Author

littledan commented Apr 5, 2019

@@ -102,6 +102,7 @@ <h1> FinalizationGroupCleanupJob ( _finalizationGroup_ [ , _callback_ ] ) </h1>
1. Perform ? Call(_callback_, *undefined*, _iterator_).
1. Return *undefined*.
</emu-alg>
<emu-note type=editor>If calling the callback throws an error, this will be caught within the RunJobs algorithm and reported to the host. HTML does not apply the RunJobs algorithm, but will also report the error.</emu-note>

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 5, 2019

Member

Isn’t html required to have the same observable effect as RunJobs, whether it follows those steps exactly or not?

This comment has been minimized.

Copy link
@mhofman

mhofman Apr 6, 2019

Contributor

That's only the case when this job is invoked from DoAgentFinalization. If invoked by FinalizationGroup.prototype.cleanupSome, the error will go up to user code normally.

This comment has been minimized.

Copy link
@littledan

littledan Apr 6, 2019

Author Member

@ljharb There's a standing disagreement about the layering. I hope we can revisit it in a future TC39 meeting, but I'm waiting for some ongoing HTML work to complete before doing that. This is just a note, and I'm doing the best I can in the face of this disagreement.

@mhofman Good point, I've clarified this.

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 6, 2019

Member

@littledan I’m aware of that, but that doesn’t mean 262 makes/should make mention of it.

This comment has been minimized.

Copy link
@littledan

littledan Apr 6, 2019

Author Member

@ljharb I hope to resolve the layering issue before Stage 4; we can remove the reference if it's not fixed in time, but I think the clarity is important.

@gsathya

gsathya approved these changes Apr 6, 2019

@littledan littledan merged commit 19e6662 into tc39:master Apr 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.