Improve worker shutdown #26628

gterzian · 2020-05-24T07:43:48Z

FIX #26548
FIX #25212

and also a step towards #26502

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #___ (GitHub issue number if applicable)

There are tests for these changes OR
These changes do not require tests because ___

highfive · 2020-05-24T07:43:53Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/worker.rs, components/script/serviceworker_manager.rs, components/script/dom/globalscope.rs, components/script/dom/dedicatedworkerglobalscope.rs, components/script/dom/bindings/trace.rs and 1 more
@KiChjang: components/script/dom/worker.rs, components/script/serviceworker_manager.rs, components/script/dom/globalscope.rs, components/script/dom/dedicatedworkerglobalscope.rs, components/script/dom/bindings/trace.rs and 1 more

highfive · 2020-05-24T07:43:55Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

gterzian · 2020-05-24T08:43:27Z

@bors-servo try=wpt


        Auto merge of #26628 - gterzian:shutdown_workers, r=<try>

[WIP] Add mechanism to join on service- and dedicated-worker threads  FIX #26548 and also a step towards #26502 ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2020-05-24T08:43:33Z

⌛ Trying commit b91fb06 with merge 981fdf3...

gterzian · 2020-05-24T14:39:26Z

@asajeffrey r?


        add mechanism to join on service- and dedicated-worker threads

jdm · 2020-05-29T15:22:39Z

@asajeffrey Review ping.

asajeffrey · 2020-05-29T15:34:58Z

components/script/dom/abstractworkerglobalscope.rs

+            // and handle the event.
+
+            // Shutdown.
+            assert!(msg.is_err());


Using an error rather than a message for signalling shutdown seems odd?

asajeffrey

LGTM apart from some nits.

asajeffrey · 2020-05-29T15:38:49Z

components/script/dom/dedicatedworkerglobalscope.rs

+    /// A receiver of control messages,
+    /// currently only used to signal shutdown.
+    #[ignore_malloc_size_of = "Channels are hard"]
+    control_receiver: Receiver<()>,


Use a ShutdownMsg type rather than ()?

The idea is that the shutdown is signalled by dropping the sender(and to avoid explicit shutdown messages), and the () is used as a placeholder since no other messages are used for now. Re the sthutdown, I'll answer below.

I also like using Never as a message type in these cases, which make it clear(I.e statically impossible) to send a message: https://github.com/rust-lang/rls/blob/17a439440e6b00b1f014a49c6cf47752ecae5bb7/rls/src/concurrency.rs#L88

It also removes the question of handling OK case on the receive side (you write an empty match: match never {})

Admittedly, “wtf is Sender<Never>“ is a common reaction when people see such code for the first time. On the flip side, once they learn the pattern, shutdown: Sender<Never> becomes an immediately recognized code shape.

I think that's a good idea, added a commit that does this...

asajeffrey · 2020-05-29T15:38:49Z

components/script/dom/globalscope.rs

+                .take()
+                .expect("No control sender to worker thread."),
+        );
+


Why not send a message rather than dropping the channel?

I tried to write this down over at #26502

While we use a lot of exit messages throughout the codebase, I think @matklad had a good point that relying on dropping channels is better, because it removes all sort of opportunities for accidental bad designs.

This pattern might not be applicable where there is a process boundary and IPC, and in this case where a thread shuts-down and then needs to shutdown it's worker threads, it seems effective, and I also like that it enforces the need for a dedicated "control" channel for parent -> child communication, whereas with an exit message it would have been tempting to just re-use one of the existing channels.

So for example, if the control channel from the "parent" thread is cloned and passed to child or peer threads somewhere, then the shut-down mechanism will fail, and that would then force us to not clone the control channel and instead introduce a new dedicated kind of channel for the communication pattern at hand.

One could say that you loose the difference between an explicit shutdown, and just a panic from the parent thread, however in practice both should result in shutdown of the worker, and since they're in the same process the panic would take everything down with it right?

Please let me know what you think of this.

My personal taste would be to use shutdown messages, and reserve dropping channels for the case where the sender has panicked. This is just taste though, it'll be the same shutdown code running in either case.

asajeffrey · 2020-05-29T15:38:49Z

components/script/dom/globalscope.rs

+            .take()
+            .expect("No handle to join on worker.")
+            .join()
+            .expect("Couldn't join on worker thread.");


Should there be a panic if joining fails?

Since we're in script, I would say yes? What do you think?

Another matter of taste, since I think the join only fails if the worker panics? Personally I'd log an error and try to carry on.

asajeffrey · 2020-05-29T15:38:49Z

components/script/serviceworker_manager.rs

+            self.control_sender
+                .take()
+                .expect("No control sender to worker thread."),
+        );


matklad · 2020-05-30T06:25:50Z

components/script/serviceworker_manager.rs

+    /// currently only used to signal shutdown.
+    control_sender: Option<Sender<()>>,
+    /// A handle to join on the worker thread.
+    join_handle: Option<JoinHandle<()>>,


There are two places in this PR where we have optional shutdown signal, optional Thread handle, and we do “take then join” in Drop. If we wrap JoinHandle into a type which joins on Drop, than we can remove options and custom code from Drop (and one custom Drop impl altogether) and replace them with // filed(drop) order is significant comment.

https://github.com/rust-analyzer/ra_vfs/blob/c4a2aaaf3c2d5231b11ee21a5f6ca34c3955805c/src/io.rs#L55

Thanks, noted, I think that's something I might look into if we end-up doing this in more places.

Personally I'm not sure relying on comments and field order is necessarily more robust than some additional explicit code like with those drop implementations...

In one of the two cases the options seem necessary because the worker thread is not created when the initial struct is created...

Yeah, I also prefer to write something than to rely on implicit drop order. What swayed me for VFS case is that I was able to remove the Option(which unfortunately infected code other than Drop). If OptIon is required anyway, manual impl indeed would be better.


        properly shutdown dedicated workers when the owning scope shuts-down


        remove messagaging and worker infra on workerscope exits


        add a control chan to workers, use to signal shutdown


        turn serviceworker event-loop back on

gterzian · 2020-06-03T04:35:42Z

@asajeffrey Ok, comments addressed, we can always discuss the various approaches further as part of #26502

asajeffrey

LGTM.

gterzian · 2020-06-03T15:43:04Z

@bors-servo r=asajeffrey

bors-servo · 2020-06-03T15:43:05Z

📌 Commit f4d258d has been approved by asajeffrey


        Auto merge of #26628 - gterzian:shutdown_workers, r=asajeffrey

Improve worker shutdown  FIX #26548 FIX #25212 and also a step towards #26502 ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2020-06-03T16:01:00Z

⌛ Testing commit f4d258d with merge 2c04c9f...

bors-servo · 2020-06-03T17:00:08Z

💔 Test failed - status-taskcluster

gterzian · 2020-06-03T17:05:01Z

@bors-servo retry

(the css issue I don't want to look-up right now)

jdm · 2020-06-03T17:05:47Z

That's #23290.

bors-servo · 2020-06-03T17:44:12Z

⌛ Testing commit f4d258d with merge ff3d5c5...

bors-servo · 2020-06-03T18:43:55Z

☀️ Test successful - status-taskcluster
Approved by: asajeffrey
Pushing ff3d5c5 to master...

highfive assigned asajeffrey May 24, 2020

highfive added the S-awaiting-review label May 24, 2020

gterzian force-pushed the gterzian:shutdown_workers branch from b91fb06 to fe75c6c May 24, 2020

gterzian marked this pull request as ready for review May 24, 2020

gterzian changed the title ~~[WIP] Add mechanism to join on service- and dedicated-worker threads~~ Add mechanism to join on service- and dedicated-worker threads May 24, 2020

add mechanism to join on service- and dedicated-worker threads

ed688fe

gterzian mentioned this pull request May 29, 2020

JS Runtime drops with live children in worker #25212

Closed

gterzian force-pushed the gterzian:shutdown_workers branch 5 times, most recently from 994b56c to db402be May 29, 2020

gterzian requested a review from asajeffrey May 29, 2020

gterzian changed the title ~~Add mechanism to join on service- and dedicated-worker threads~~ Improve worker shutdown May 29, 2020

gterzian force-pushed the gterzian:shutdown_workers branch from db402be to 82adfd9 May 29, 2020

asajeffrey reviewed May 29, 2020

View changes

components/script/dom/abstractworkerglobalscope.rs Outdated

// and handle the event.

// Shutdown.

assert!(msg.is_err());

This comment has been minimized.

Sign in to view

asajeffrey May 29, 2020
Member

Using an error rather than a message for signalling shutdown seems odd?

asajeffrey requested changes May 29, 2020

View changes

matklad reviewed May 30, 2020

View changes

gterzian force-pushed the gterzian:shutdown_workers branch 2 times, most recently from 40bbd5e to 45c3ca9 May 30, 2020

gterzian added 3 commits May 29, 2020

properly shutdown dedicated workers when the owning scope shuts-down

6f34b52

remove messagaging and worker infra on workerscope exits

Loading status checks…

f4d258d

add a control chan to workers, use to signal shutdown

947fa8b

turn serviceworker event-loop back on

5174739

gterzian force-pushed the gterzian:shutdown_workers branch from 45c3ca9 to f4d258d Jun 3, 2020

asajeffrey approved these changes Jun 3, 2020

View changes

highfive added S-awaiting-merge and removed S-awaiting-review labels Jun 3, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Jun 3, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Jun 3, 2020

bors-servo merged commit ff3d5c5 into servo:master Jun 3, 2020
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jun 3, 2020

gterzian deleted the gterzian:shutdown_workers branch Jun 4, 2020

servo / servo

Improve worker shutdown #26628

Improve worker shutdown #26628

gterzian commented May 24, 2020 •

edited

highfive commented May 24, 2020

highfive commented May 24, 2020

gterzian commented May 24, 2020

bors-servo commented May 24, 2020

gterzian commented May 24, 2020

jdm commented May 29, 2020

This comment has been minimized.

asajeffrey left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gterzian commented Jun 3, 2020

asajeffrey left a comment

gterzian commented Jun 3, 2020

bors-servo commented Jun 3, 2020

bors-servo commented Jun 3, 2020

bors-servo commented Jun 3, 2020

gterzian commented Jun 3, 2020

jdm commented Jun 3, 2020

bors-servo commented Jun 3, 2020

bors-servo commented Jun 3, 2020

servo / servo

Join GitHub today

Improve worker shutdown #26628

Improve worker shutdown #26628

Conversation

gterzian commented May 24, 2020 • edited

highfive commented May 24, 2020

highfive commented May 24, 2020

gterzian commented May 24, 2020

bors-servo commented May 24, 2020

gterzian commented May 24, 2020

jdm commented May 29, 2020

This comment has been minimized.

asajeffrey left a comment

This comment has been minimized.

This comment has been minimized.

gterzian May 30, 2020 • edited

This comment has been minimized.

matklad May 30, 2020 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gterzian May 30, 2020 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

matklad May 30, 2020 • edited

This comment has been minimized.

gterzian May 30, 2020 • edited

This comment has been minimized.

gterzian commented Jun 3, 2020

asajeffrey left a comment

gterzian commented Jun 3, 2020

bors-servo commented Jun 3, 2020

bors-servo commented Jun 3, 2020

bors-servo commented Jun 3, 2020

gterzian commented Jun 3, 2020

jdm commented Jun 3, 2020

bors-servo commented Jun 3, 2020

bors-servo commented Jun 3, 2020

gterzian commented May 24, 2020 •

edited

gterzian May 30, 2020 •

edited

matklad May 30, 2020 •

edited

gterzian May 30, 2020 •

edited

matklad May 30, 2020 •

edited

gterzian May 30, 2020 •

edited