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

Clarify explicitly when promises are to be resolved #461

Closed
jrummell-chromium opened this issue Dec 10, 2019 · 2 comments · Fixed by #538
Closed

Clarify explicitly when promises are to be resolved #461

jrummell-chromium opened this issue Dec 10, 2019 · 2 comments · Fixed by #538
Assignees
Labels
Milestone

Comments

@jrummell-chromium
Copy link
Contributor

Currently the spec notes "Since promise handlers are queued as microtasks, these will be executed ahead of any events queued by the preceding steps" in several places. For example, in generateRequest:

  1. Queue a task to run the following steps:
  1. If any of the preceding steps failed, reject promise with a new DOMException whose name is the appropriate error name.
  2. Set the sessionId attribute to session id.
  3. Set this object's callable value to true.
  4. Run the Queue a "message" Event algorithm on the session, providing message type and message.
  5. Resolve promise.

NOTE: Since promise handlers are queued as microtasks, these will be executed ahead of any events queued by the preceding steps.

To make the spec clearer, it would be better to indicate exactly when the promise is resolved, compared to other steps. The example above would switch 4 and 5 to become:

  1. Resolve promise.
  2. Run the Queue a "message" Event algorithm on the session, providing message type and message.

And then the note is not needed, as it is now clear that the promise should be resolved before the "message" event is generated (and does not depend on promise handlers being implemented as microtasks). This also appears to be the suggestion in #19 (comment) although it was switched back when addressing #14.

Resolving the promise before firing an event is used in other specs (e.g. Presentation API).

There is a potential problem with this in close(). Currently the attribute closed promise is resolved before the close() method promise is resolved. This will break the WPT tests for playback-temporary-events as it checks that the attribute promise is resolved before the close() method promise is resolved.

@mounirlamouri mounirlamouri added this to the V2 milestone Sep 8, 2020
@joeyparrish joeyparrish modified the milestones: V2, V2-Bugfixes Sep 8, 2020
@joeyparrish joeyparrish modified the milestones: V2-Bugfixes, V2 Mar 22, 2023
@joeyparrish
Copy link
Member

NOTE: Since promise handlers are queued as microtasks, these will be executed ahead of any events queued by the preceding steps.

To make the spec clearer, it would be better to indicate exactly when the promise is resolved, compared to other steps. The example above would switch 4 and 5 to become:

  1. Resolve promise.
  2. Run the Queue a "message" Event algorithm on the session, providing message type and message.

And then the note is not needed, as it is now clear that the promise should be resolved before the "message" event is generated (and does not depend on promise handlers being implemented as microtasks).

I don't think this matters so much. It's arbitrary, IMO. That promises are microtasks is not something we depend on, so much as a fact of the web environment. (I recall this being part of a low-level spec for the web, but I haven't been able to find a reference to back this up. I could be wrong.)

The introduction of Promises into this spec happened at a time when few of us were deeply familiar with Promises and their underlying details, though, so I can understand why there has been some confusion.

It's easy to swap those steps, and it shouldn't hurt anything.

Resolving the promise before firing an event is used in other specs (e.g. Presentation API).

I'm all for consistency.

There is a potential problem with this in close(). Currently the attribute closed promise is resolved before the close() method promise is resolved. This will break the WPT tests for playback-temporary-events as it checks that the attribute promise is resolved before the close() method promise is resolved.

We probably could and should fix wpt not to care which order those two promises are resolved in, so long as they are in fact both resolved. They should be equivalent promises for all intents and purposes.


So I'll flip the order of those two steps, and I'll work on a PR for WPT to make the test more flexible.

@joeyparrish
Copy link
Member

joeyparrish commented May 14, 2024

EME spec PR to reorder steps: #538

WPT PR to make the test more flexible WRT closed: web-platform-tests/wpt#46268 (Edit: now merged)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants