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

Resolve promises before enqueuing events #538

Merged
merged 5 commits into from
Jun 18, 2024
Merged

Conversation

joeyparrish
Copy link
Member

@joeyparrish joeyparrish commented May 14, 2024

Closes #461


Preview | Diff

@joeyparrish
Copy link
Member Author

@jrummell-chromium JFYI

Copy link
Member

@chrisn chrisn left a comment

Choose a reason for hiding this comment

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

@jernoble Does this look OK to you, any potential interop concerns?

Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

I don’t think this is correct. Promises need to be resolved in task queues also. Also, promises should resolve with something even if that’s an explicit undefined.

@joeyparrish
Copy link
Member Author

Promises need to be resolved in task queues also.

I don't understand what you mean by this. Can you suggest an edit to show me what language you would agree with?

@marcoscaceres
Copy link
Member

Sorry for the delay:

For example, from the Fetch spec:

  1. Queue a fetch task to run the following steps, with fetchParams’s task destination.
    1.1 Pull from bytes buffer into stream.
    1.1 If stream is errored, then terminate fetchParams’s controller.
    1.1 Resolve promise with undefined.

See also https://www.w3.org/TR/web-share/ ... how the global task queue is used to resolve promises.

@joeyparrish
Copy link
Member Author

I audited every instance of "resolve" or "resolved" in the spec. All have values, and all are now inside either "Run the following steps in parallel" or "queue a task" or "Use the cdm to execute". (I had to fix only one instance of a missing queue, in the recently-introduced method getStatusForPolicy().)

@joeyparrish
Copy link
Member Author

Also worth a note, since we're being thorough: we resolve a promise inside the session closed algorithm, which is always run inside a task or in a set of parallel steps.

@joeyparrish
Copy link
Member Author

I'm ready to merge this if there is no further feedback.

I'll be on leave for five weeks starting June 22, so if a positive consensus is reached after that date, please don't wait for me to hit the "merge" button.

@joeyparrish joeyparrish merged commit 76d2ed6 into main Jun 18, 2024
1 check passed
@joeyparrish joeyparrish deleted the clarify-promise-order branch June 18, 2024 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify explicitly when promises are to be resolved
4 participants