editorial: Tidy up uses of "in parallel". #299

rakuco · 2021-02-05T13:38:05Z

Steps that run "in parallel" need to take several extra restrictions into
account: we do not have access to documents or global objects, cannot create
IDL interfaces or manipulate promises, for example, all of which we had been
doing, along with needlessly running nested "in parallel" steps.

This change attempts to rectify the situation by only running things in
parallel when absolutely necessary (i.e. when reaching out to the OS), and
queuing global tasks (emphasis on "global" given whatwg/html#4980 and
whatwg/html#5653) from there when we need to manipulate promises or create
objects.

"Obtain permission" algorithm:

Stop running in parallel. Callers should be responsible for choosing
whether it should be run in parallel or not.

WakeLock.request():

Separate returning a promise and running steps in parallel. This style is
more usual.
Refer to the "relevant settings object" rather than the "current settings
object", as we are inside a method of an IDL interface and can rely on it
being defined. I do not think this is a user-visible change, and it looks
cleaner.
Queue a global task to reject |promise| when the permission request run
in parallel is denied.
Manipulate the [[ActiveLocks]] internal slot, check page visibility,
invoke "acquire a wake lock", create a new WakeLockSentinel and resolve
the returned promise in a queued global task.

WakeLockSentinel.release():

Do not run the "release a wake lock" algorithm in parallel (see the
changes to the algorithm itself below).
Just return a resolved promise once the rest of the steps run. The
returned promise does not have much use, but has been kept to avoid
breaking API compatibility.
One user-visible consequence is that the "release" event is fired
synchronously and before the function returns.

"Acquire wake lock" algorithm:

Stop running everything in parallel.
Move all [[ActiveLocks]] manipulation to WakeLock.request() itself,
and make the algorithm just ask the platform for a wake lock.

"Release wake lock" algorithm:

Stop running everything in parallel. The only steps that still need to run
in parallel are the ones asking the platform to release the wake lock.
Consequently, stop queueing a task to change [[Released]] and fire the
"release" event, and do it directly in the algorithm instead.

Big thanks to Anne van Kesteren, Domenic Denicola and Marcos Cáceres for
their input.

Fixes #293.

Open items:

How does one check if |document| is hidden in WakeLock.request()'s parallel steps? Or, more generally speaking, how can we avoid reaching the steps where we ask the platform for a lock if the page was hidden, for example?
WakeLock.request(): does it make sense to have to separate "in parallel" steps to first acquire a platform lock and then manipulate [[ActiveLocks]]?
WakeLock.request(): does manipulating [[ActiveLocks]] from a queued task leave the algorithm open to race conditions?
WakeLockSentinel.release(): the Web Platform Design Principles say "events should fire before Promises resolve". Do the changes make sense here? We are changing [[Released]] and firing the "released" event "synchronously" in the event loop before even creating the resolved promise we are returning. This feels a bit odd, but on the other hand I'm not sure how to avoid running the "release wake lock" algorithm in parallel if we create the promise first.
- This one of the things that led to "Release a wake lock": should we queue a task to fire the "release" event? #293: if we queue a task to fire the event, it might not be delivered at all. Is that OK? Should [[Released]] be changed synchronously though, so that e.g. when we are navigating to another page, should .released is true in the "visibilitychange" event handler even though the "release" event has not been dispatched?

The following tasks have been completed:

Modified Web platform tests (screen-wake-lock: Adapt to https://github.com/w3c/screen-wake-lock/pull/299 web-platform-tests/wpt#28148)

Implementation commitment:

Chromium (https://bugs.chromium.org/p/chromium/issues/detail?id=1190110)
WebKit - Not participating in this working group.

Preview | Diff

rakuco · 2021-02-05T13:39:18Z

@annevk @domenic this is a first attempt to tackle #293 after the discussion we've had there. I'd appreciate it if you could see if this makes sense and look at the "open items" in the PR message with things that are still unclear to me.

index.html

reillyeon · 2021-02-05T21:18:19Z

While you are here fixing the "in parallel" sections this specification should be updated to define a task source onto which the tasks returning to main thread are queued.

rakuco · 2021-02-06T13:38:31Z

While you are here fixing the "in parallel" sections this specification should be updated to define a task source onto which the tasks returning to main thread are queued.

Do you think it's better to come up with a new task source, or would it make sense to use the user interaction task source?

rakuco · 2021-02-08T10:02:36Z

Do you think it's better to come up with a new task source, or would it make sense to use the user interaction task source?

Per w3c/presentation-api#360 and w3ctag/design-principles#38 it looks like it makes more sense to define another task source.

rakuco · 2021-02-11T08:34:07Z

@annevk @domenic friendly ping, it'd be great if you could take a look at the proposed changes here and/or the questions in the PR message which I couldn't answer yet.

annevk · 2021-02-11T09:56:06Z

Some thoughts:

obtain permission shouldn't run in parallel, it should be invoked from in parallel, but it doesn't make sense for it itself to then also go in parallel again (in parallel steps cannot return things)
A slightly clearer style is to say "run these steps in parallel" and then have a subsequent step return the promise.
You really want to use "queue a global task" and always pass it the global and task source to use explicitly. That's the way forward.

In parallel usage otherwise seems reasonable from a quick skim. (If you're still not too sure about it, HTML has a bunch of guidance language for it, it's basically akin to a thread/process. Also happy to chat at https://whatwg.org/irc or equivalent.)

Questions:

I'm not sure about hidden documents. There are some issues about this against HTML as well. (Checking the hidden attribute is definitely wrong though. You don't want to invoke IDL members in your algorithms as a general rule.)
I'm not sure I understand this.
Yeah, it seems "acquire a wake lock" could be run twice if you invoke the method twice.
The order of event/promise in release() seems fine. It might be clearer to run "release a wake lock" when the slot is false and then have a single return step at the end.
- If you don't want release() to fire an event directly (though not sure if it matters much), you could make it queue a task to run "release a wake lock" and resolve the promise with undefined. That way other callers of "release a wake lock" would still dispatch the event in whatever task they find themselves in.

rakuco · 2021-02-11T14:48:15Z

Thanks for the review, @annevk. I've pushed some new commits addressing some of the points you've brought up.

Some thoughts:

obtain permission shouldn't run in parallel, it should be invoked from in parallel, but it doesn't make sense for it itself to then also go in parallel again (in parallel steps cannot return things)

I was hoping to get rid of the algorithm altogether in #298, but in the meantime I've dropped the "in parallel" bit in this PR.

A slightly clearer style is to say "run these steps in parallel" and then have a subsequent step return the promise.

Done.

You really want to use "queue a global task" and always pass it the global and task source to use explicitly. That's the way forward.

Is there any particular reason for using "global" here? I recently defined a task source in #301 and was using the regular "queue a task" language with it.

Questions:

I'm not sure about hidden documents. There are some issues about this against HTML as well. (Checking the hidden attribute is definitely wrong though. You don't want to invoke IDL members in your algorithms as a general rule.)

Hmm, I wonder if the algorithms in "handling document loss of {full activity, visibility}" could set some flag that could be checked in the parallel steps of WakeLock.request(). Some sort of "abort when some-flag is set // if aborted reject promise".

Yeah, it seems "acquire a wake lock" could be run twice if you invoke the method twice.

Would it be OK (from an idiomatic perspective) to remove the check altogether and sort of under-specify the behavior by leaving it up to each implementation to determine whether to ask the platform for a lock?

The order of event/promise in release() seems fine. It might be clearer to run "release a wake lock" when the slot is false and then have a single return step at the end.

Thanks, done.

If you don't want release() to fire an event directly (though not sure if it matters much), you could make it queue a task to run "release a wake lock" and resolve the promise with undefined. That way other callers of "release a wake lock" would still dispatch the event in whatever task they find themselves in.

I'm trying to determine if there's some general guidance here. If it's basically "fire events away and only queue a task to fire them if necessary", then I'm fine with the changes being done here.

annevk · 2021-02-11T16:00:05Z

Is there any particular reason for using "global" here?

Yes, it helps ensure the task is queued on the correct event loop and all the relevant bits are set correctly. (Though @domenic has been suggesting that we have some kind of ambient global available for constructing objects we could also repurpose here perhaps...)

Would it be OK (from an idiomatic perspective) to remove the check altogether and sort of under-specify the behavior by leaving it up to each implementation to determine whether to ask the platform for a lock?

Maybe, not sure. It seems easy enough to add some boolean that you can use to prevent this, no?

If it's basically "fire events away and only queue a task to fire them if necessary", then I'm fine with the changes being done here.

It depends on the cause and the target, I think here it's fine. If it was in response to a DOM mutation I'd ask for a change.

@annevk

* @annevk says in w3c#299 that, as a general rule, invoking IDL members from algorithms, like we were doing with `document.hidden`, is not recommended. Instead, invoke the "determine the visibility state" steps defined in the Page Visibility spec directly. * Clean up the "Dependencies" section; for now we only need one term that is not exported by the latest Page Visibility TR (this is fixed in the ED): the "now hidden algorithm". The steps described in the "Handling document loss of visibility" steps now explicitly run after that algorithm, which runs when the top-level browsing context's Document becomes hidden. More generally, these changes should also make it more clear that the visibility state checks only apply to the top-level browsing context (according to the Page Visibility spec itself). Although we were previously checking if a given `Document` was `hidden` even if it did not belong to a top-level browsing context, that was simply invalid.

rakuco · 2021-02-15T14:20:39Z

@annevk I've pushed a few more commits. I think I've managed to solve everything, but I am not entirely sure doing things from a queued global task is enough to avoid race conditions and not being allowed to refer to global objects:

Switched to "queue a global task"
Switched to manipulating the [[ActiveLocks]] list only in the main event loop (from a queued global task in WakeLock.request() and without doing anything special in the "Release a wake lock" algorithm)
I think I've solved the page visibility issue by checking it inside the queued global task in WakeLock.request() (I'm adjusting the wording more broadly in editorial: Improve references to terms from the Page Visibility spec. #306).

marcoscaceres

Some drive by feedback...

index.html

@annevk

…#306) * @annevk says in #299 that, as a general rule, invoking IDL members from algorithms, like we were doing with `document.hidden`, is not recommended. Instead, invoke the "determine the visibility state" steps defined in the Page Visibility spec directly. * Clean up the "Dependencies" section; for now we only need one term that is not exported by the latest Page Visibility TR (this is fixed in the ED): the "now hidden algorithm". The steps described in the "Handling document loss of visibility" steps now explicitly run after that algorithm, which runs when the top-level browsing context's Document becomes hidden. More generally, these changes should also make it more clear that the visibility state checks only apply to the top-level browsing context (according to the Page Visibility spec itself). Although we were previously checking if a given `Document` was `hidden` even if it did not belong to a top-level browsing context, that was simply invalid.

kenchris · 2021-02-25T14:17:27Z

index.html

          <li>Let |document:Document| be the [=environment settings object /
-          responsible document=] of the <a>current settings object</a>.
+          responsible document=] of <a>this</a>' <a>relevant settings
+          object</a>.


Domenic just used "
Let |document:Document| be the [=current settings object=]'s [=associated Document=]."

I think it makes sense to clean this up in #307 rather than here, as it's part of the effort to stop mentioning "responsible document" altogether.

rakuco · 2021-02-25T14:50:04Z

@annevk @domenic @kenchris @marcoscaceres I think this PR is finally ready. All uses of "in parallel" have been audited, I think WakeLock.request() is free of race conditions when manipulating [[ActiveLocks]] and we are not checking anything from the main event loop when in parallel. The questions that seems to remain is there should be a queued task to fire the "release" event and change [[Released]] to true, which is what motivated #293 in the first place. My understanding based on this comment above is that in this case there is no need to queue a task and we can do things directly instead.

There are further cleanups that @domenic is doing in #307 related to the use of "responsible document"; I think it makes sense to land them separately there.

domenic

Looking very solid; just some minor issues with nesting.

index.html

@domenic

Incorporate suggestions made by @domenic in w3c#299: * Steps 1.1 and 1.2 which checked `document` should not be substeps of step 1, but siblings just like other subsequent steps that also check `document`. * Checking the value of `state` should be done at the same nesting level of the step that sets `state`, not as a nested substep. These changes should have no user-visible effects.

@domenic

Incorporate suggestions made by @domenic in w3c#299: * Steps 1.1 and 1.2 which checked `document` should not be substeps of step 1, but siblings just like other subsequent steps that also check `document`. * Checking the value of `state` should be done at the same nesting level of the step that sets `state`, not as a nested substep. These changes should have no user-visible effects.

@domenic

This is the preferred form as found e.g. in the Web IDL and Fetch specs. Originally spotted by @domenic while reviewing w3c#299.

@domenic

This is the preferred form as found e.g. in the Web IDL and Fetch specs. Originally spotted by @domenic while reviewing #299.

@domenic

…#308) Incorporate suggestions made by @domenic in #299: * Steps 1.1 and 1.2 which checked `document` should not be substeps of step 1, but siblings just like other subsequent steps that also check `document`. * Checking the value of `state` should be done at the same nesting level of the step that sets `state`, not as a nested substep. These changes should have no user-visible effects.

marcoscaceres

LGTM.

marcoscaceres · 2021-03-18T03:14:36Z

@rakuco, any blockers or anything you need help with here?

rakuco · 2021-03-18T10:30:53Z

@rakuco, any blockers or anything you need help with here?

Nope, thanks a lot for reviewing and accepting the PR, and sorry I haven't merged it yet. I've been dragged into other stuff but will try to check if any tests need to be adjusted and merge it today or tomorrow.

marcoscaceres · 2021-03-18T23:51:27Z

Great to hear! thanks for the update, @rakuco!

Besides tidying up uses of "in parallel" in algorithms, that spec change also makes the "release wake lock" algorithm fire the "release" event and change the `[[Released]]` slot immediately rather than in a queued task. Adjust some tests to make sure the new behavior is properly verified here.

Steps that run "in parallel" need to take several extra restrictions into account: we do not have access to documents or global objects, cannot create IDL interfaces or manipulate promises, for example, all of which we had been doing, along with needlessly running nested "in parallel" steps. This change attempts to rectify the situation by only running things in parallel when absolutely necessary (i.e. when reaching out to the OS), and queuing global tasks (emphasis on "global" given whatwg/html#4980 and whatwg/html#5653) from there when we need to manipulate promises or create objects. "Obtain permission" algorithm: * Stop running in parallel. Callers should be responsible for choosing whether it should be run in parallel or not. `WakeLock.request()`: * Separate returning a promise and running steps in parallel. This style is more usual. * Refer to the "relevant settings object" rather than the "current settings object", as we are inside a method of an IDL interface and can rely on it being defined. I do not think this is a user-visible change, and it looks cleaner. * Queue a global task to reject `|promise|` when the permission request run in parallel is denied. * Manipulate the `[[ActiveLocks]]` internal slot, check page visibility, invoke "acquire a wake lock", create a new WakeLockSentinel and resolve the returned promise in a queued global task. `WakeLockSentinel.release()`: * Do not run the "release a wake lock" algorithm in parallel (see the changes to the algorithm itself below). * Just return a resolved promise once the rest of the steps run. The returned promise does not have much use, but has been kept to avoid breaking API compatibility. * One user-visible consequence is that the "release" event is fired synchronously and before the function returns. "Acquire wake lock" algorithm: * Stop running everything in parallel. * Move all `[[ActiveLocks]]` manipulation to `WakeLock.request()` itself, and make the algorithm just ask the platform for a wake lock. "Release wake lock" algorithm: * Stop running everything in parallel. The only steps that still need to run in parallel are the ones asking the platform to release the wake lock. * Consequently, stop queueing a task to change `[[Released]]` and fire the "release" event, and do it directly in the algorithm instead. Big thanks to Anne van Kesteren, Domenic Denicola and Marcos Cáceres for their input. Fixes w3c#293.

rakuco · 2021-03-22T11:43:22Z

I'd like to thank everyone once again for reviewing this patch and for all the discussions. I've updated the PR message and included links to the Chromium bug and WPT PR that this change has originated. Since there have been no objections and the PR in WPT is close to being merged, I'm finally going to merge this one.

Besides tidying up uses of "in parallel" in algorithms, that spec change also makes the "release wake lock" algorithm fire the "release" event and change the `[[Released]]` slot immediately rather than in a queued task. Adjust some tests to make sure the new behavior is properly verified here.

…ke-lock#299, a=testonly Automatic update from web-platform-tests screen-wake-lock: Adapt to w3c/screen-wake-lock#299 (#28148) Besides tidying up uses of "in parallel" in algorithms, that spec change also makes the "release wake lock" algorithm fire the "release" event and change the `[[Released]]` slot immediately rather than in a queued task. Adjust some tests to make sure the new behavior is properly verified here. -- wpt-commits: 36aa2a490d8aabdb15436e62910fe87111fddbdf wpt-pr: 28148

This spec change reworked the way "in parallel" is used in normative text. In terms of implementation, the only thing that needs to change is the way we set WakeLockSentinel::released_ and fire the "release" event. This is now done synchronously rather than from a queued task, which means WakeLockSentinel.release() will immediately perform those actions rather than return a promise first. The promise is not really necessary, but it has been kept in the spec to avoid making a backwards-incompatible change. The corresponding test changes are being landed directly in WPT and once they are imported the new -expected.txt will be removed automatically: web-platform-tests/wpt#28148 Bug: 1190110 Change-Id: Ia9fc0174d36f70a7f7c8ede90c5992b07e1c69ec Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2778411 Reviewed-by: Reilly Grant <[email protected]> Commit-Queue: Raphael Kubo da Costa <[email protected]> Cr-Commit-Position: refs/heads/master@{#867272}

This spec change reworked the way "in parallel" is used in normative text. In terms of implementation, the only thing that needs to change is the way we set WakeLockSentinel::released_ and fire the "release" event. This is now done synchronously rather than from a queued task, which means WakeLockSentinel.release() will immediately perform those actions rather than return a promise first. The promise is not really necessary, but it has been kept in the spec to avoid making a backwards-incompatible change. The corresponding test changes are being landed directly in WPT and once they are imported the new -expected.txt will be removed automatically: web-platform-tests/wpt#28148 Bug: 1190110 Change-Id: Ia9fc0174d36f70a7f7c8ede90c5992b07e1c69ec Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2778411 Reviewed-by: Reilly Grant <[email protected]> Commit-Queue: Raphael Kubo da Costa <[email protected]> Cr-Commit-Position: refs/heads/master@{#867272} GitOrigin-RevId: dcfc63084215ecd3292e911eefe07fddcf30aed0

rakuco requested review from reillyeon and kenchris February 5, 2021 13:38

kenchris reviewed Feb 5, 2021

View reviewed changes

index.html Outdated Show resolved Hide resolved

kenchris reviewed Feb 5, 2021

View reviewed changes

index.html Outdated Show resolved Hide resolved

rakuco force-pushed the in-parallel-rework branch from da7e759 to 8dc5d6a Compare February 11, 2021 14:37

rakuco mentioned this pull request Feb 15, 2021

editorial: Improve references to terms from the Page Visibility spec. #306

Merged

2 tasks

rakuco force-pushed the in-parallel-rework branch from 8dc5d6a to 56b7230 Compare February 15, 2021 14:09

rakuco requested a review from marcoscaceres February 17, 2021 17:43

marcoscaceres reviewed Feb 22, 2021

View reviewed changes

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

index.html Show resolved Hide resolved

rakuco mentioned this pull request Feb 24, 2021

Clean up task queuing and document management #307

Closed

rakuco force-pushed the in-parallel-rework branch from 6e19ace to 2074bdb Compare February 25, 2021 13:36

kenchris reviewed Feb 25, 2021

View reviewed changes

rakuco marked this pull request as ready for review February 25, 2021 14:46

domenic reviewed Feb 26, 2021

View reviewed changes

rakuco mentioned this pull request Feb 27, 2021

editorial: Adjust nesting levels in the WakeLock.request() algorithm. #308

Merged

rakuco added a commit to rakuco/wake-lock that referenced this pull request Feb 27, 2021

editorial: Say "this's" instead of "this'".

b8ad60d

This is the preferred form as found e.g. in the Web IDL and Fetch specs. Originally spotted by @domenic while reviewing w3c#299.

rakuco mentioned this pull request Feb 27, 2021

editorial: Say "this's" instead of "this'". #309

Merged

rakuco added a commit that referenced this pull request Feb 27, 2021

editorial: Say "this's" instead of "this'". (#309)

d657c68

This is the preferred form as found e.g. in the Web IDL and Fetch specs. Originally spotted by @domenic while reviewing #299.

rakuco force-pushed the in-parallel-rework branch from c69a539 to c87c077 Compare February 28, 2021 14:10

marcoscaceres approved these changes Mar 8, 2021

View reviewed changes

rakuco force-pushed the in-parallel-rework branch from c87c077 to 7ac57c1 Compare March 22, 2021 11:42

rakuco merged commit 1e380dc into w3c:gh-pages Mar 22, 2021

rakuco deleted the in-parallel-rework branch March 22, 2021 11:44

anssiko mentioned this pull request Mar 24, 2021

Devices and Sensors WG - 2021 Q2 virtual meeting w3c/devicesensors-wg#39

Closed

rakuco mentioned this pull request Aug 20, 2021

WakeLock and only require activation once #139

Open

rakuco mentioned this pull request Sep 27, 2022

Remove permission check #326

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

editorial: Tidy up uses of "in parallel". #299

editorial: Tidy up uses of "in parallel". #299

rakuco commented Feb 5, 2021 •

edited by pr-preview bot

Loading

rakuco commented Feb 5, 2021

reillyeon commented Feb 5, 2021

rakuco commented Feb 6, 2021

rakuco commented Feb 8, 2021

rakuco commented Feb 11, 2021

annevk commented Feb 11, 2021

rakuco commented Feb 11, 2021

annevk commented Feb 11, 2021

rakuco commented Feb 15, 2021

marcoscaceres left a comment

kenchris Feb 25, 2021

rakuco Feb 25, 2021

rakuco commented Feb 25, 2021

domenic left a comment

marcoscaceres left a comment

marcoscaceres commented Mar 18, 2021

rakuco commented Mar 18, 2021

marcoscaceres commented Mar 18, 2021

rakuco commented Mar 22, 2021

editorial: Tidy up uses of "in parallel". #299

editorial: Tidy up uses of "in parallel". #299

Conversation

rakuco commented Feb 5, 2021 • edited by pr-preview bot Loading

Open items:

rakuco commented Feb 5, 2021

reillyeon commented Feb 5, 2021

rakuco commented Feb 6, 2021

rakuco commented Feb 8, 2021

rakuco commented Feb 11, 2021

annevk commented Feb 11, 2021

rakuco commented Feb 11, 2021

annevk commented Feb 11, 2021

rakuco commented Feb 15, 2021

marcoscaceres left a comment

Choose a reason for hiding this comment

kenchris Feb 25, 2021

Choose a reason for hiding this comment

rakuco Feb 25, 2021

Choose a reason for hiding this comment

rakuco commented Feb 25, 2021

domenic left a comment

Choose a reason for hiding this comment

marcoscaceres left a comment

Choose a reason for hiding this comment

marcoscaceres commented Mar 18, 2021

rakuco commented Mar 18, 2021

marcoscaceres commented Mar 18, 2021

rakuco commented Mar 22, 2021

rakuco commented Feb 5, 2021 •

edited by pr-preview bot

Loading