Restructure content process shutdown ack with threaded sender #23972

gterzian · 2019-08-15T07:10:10Z

Recycling some work from #23909

./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 ___

This change is

highfive · 2019-08-15T07:10:15Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/htmliframeelement.rs, components/constellation/pipeline.rs, components/script/dom/windowproxy.rs, components/script/script_thread.rs
@cbrewster: components/constellation/pipeline.rs
@KiChjang: components/script/dom/htmliframeelement.rs, components/script/dom/windowproxy.rs, components/script/script_thread.rs, components/script_traits/lib.rs

highfive · 2019-08-15T07:10:18Z

Warning

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

gterzian · 2019-08-15T07:12:42Z

This is another recycled commit from the big ipc experiment over at #23909

In this case, it restructures the "wait for content to shutdown" pattern, in the following way:

Since it's only used in multiprocess mode, and the "waiting" is done from the "main thread" in the content process, it seems like a threaded channel, created at the point of use, will suffice.
I remove the "wait on layout" part, since the script-thread will itself wait on layout to shutdown each time it shuts down a layout thread, so it seems to me the main-thread only needs to wait on script?
Since dropping the only sender in existence for that channel will already wake-up the receiver, it seems unnecessary to send an explicit message. Instead, we just store the sender on the script-thread, and it will drop along with it when it exits and notify the main thread(of the content-process).

@asajeffrey r?

jdm · 2019-08-15T16:24:28Z

r? @asajeffrey

asajeffrey

I'm a bit confused by these changes. Replacing an IPC channel by a crossbeam channel IO can understand, since the channel never leaves the process. But AFAICT the channel is being used as a latch, we're never actually sending a message on it?

asajeffrey · 2019-08-15T16:47:10Z

components/constellation/pipeline.rs

+            let signal = content_process_shutdown_port.recv();
+            // When the script-thread drops the sender,
+            // we expect to receive an Err(_).
+            assert!(signal.is_err());


Perhaps an error! log rather than a panic?

Oh yeah, forgot this is in the constellation thread...

It's not in the constellation thread actually, but I guess an error is better.

asajeffrey · 2019-08-15T16:47:10Z

components/constellation/pipeline.rs

+            let signal = content_process_shutdown_port.recv();
+            // When the script-thread drops the sender,
+            // we expect to receive an Err(_).
+            assert!(signal.is_err());


I'm confused about who sends on this channel, since the only uses of send(()) appear to have been deleted. Are we using a channel just for its Drop implementation?

Yes, previously there was an explicit sending of a message when the script-thread exits, which I can bring back if you think that's clearer. Or we can effectively wait until the channel drops, and assert it is indeed the Err that would be received in such a case.

I think we can count on the channel not sending any spurious errors.

Actually, I think you're right it's better to send an explicit message, since the drop could also be the result of an unexpected shutdown of the script-thread, while a message will almost always mean there was a graceful shutdown...

asajeffrey · 2019-08-15T16:47:10Z

components/script/script_thread.rs

@@ -744,6 +744,7 @@ impl ScriptThreadFactory for ScriptThread {
                thread_state::initialize(ThreadState::SCRIPT);
                PipelineNamespace::install(state.pipeline_namespace_id);
                TopLevelBrowsingContextId::install(state.top_level_browsing_context_id);
+


Don't think we need a new blank line :)

gterzian · 2019-08-15T22:17:24Z

Ok, comments addressed @asajeffrey

asajeffrey

OK, a question, but otherwise LGTM.

asajeffrey · 2019-08-16T14:45:57Z

components/constellation/pipeline.rs

@@ -545,6 +530,7 @@ impl UnprivilegedPipelineContent {
            self.script_chan.clone(),
            self.load_data.url.clone(),
        );
+        let (content_process_shutdown_chan, content_process_shutdown_port) = unbounded();


Should this be unbounded or a one-shot channel?

I'm not aware of crossbeam having a dedicated one-shot variant.

I meant bounded(1).

for what reason?

Possible space efficiency, but it probably doesn't matter that much.

I see what you mean, counterintuitively, the unbounded variant appears to require less bookkeeping than the bounded variant.

asajeffrey · 2019-08-17T01:15:36Z

@bors-servo r+

bors-servo · 2019-08-17T01:15:37Z

📌 Commit 4cd2730 has been approved by asajeffrey

bors-servo · 2019-08-17T03:00:25Z

⌛ Testing commit 4cd2730 with merge e0267e8...


        Auto merge of #23972 - gterzian:cleanup_content_process_shutdown_ack,…

… r=asajeffrey Restructure content process shutdown ack with threaded sender Recycling some work from #23909  ---  - [ ] `./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 ___    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23972)

bors-servo · 2019-08-17T05:37:32Z

💔 Test failed - status-taskcluster


        restructure content process shutdown ack with threaded sender, withou…

…t layout

gterzian · 2019-08-17T14:04:00Z

@asajeffrey Ok that was layout_2020 that required updating, done...

asajeffrey · 2019-08-17T14:42:16Z

@bors-servo r+

bors-servo · 2019-08-17T14:42:16Z

📌 Commit 8482d2e has been approved by asajeffrey

bors-servo · 2019-08-17T16:24:57Z

⌛ Testing commit 8482d2e with merge 0ff5dc5...


        Auto merge of #23972 - gterzian:cleanup_content_process_shutdown_ack,…

… r=asajeffrey Restructure content process shutdown ack with threaded sender Recycling some work from #23909  ---  - [ ] `./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 ___    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23972)

bors-servo · 2019-08-17T19:19:53Z

💔 Test failed - status-taskcluster

gterzian · 2019-08-17T19:33:26Z

@bors-servo retry

seems like a taskcluster error: TASK FAILURE during artifact upload: file-missing-on-worker: Could not read file '/Users/worker/tasks/task_1566064170/repo/intermittents.log'

bors-servo · 2019-08-17T21:33:01Z

💣 Failed to start rebuilding: Unknown error

bors-servo · 2019-08-17T21:33:05Z

⌛ Testing commit 8482d2e with merge a8ae0e5...


        Auto merge of #23972 - gterzian:cleanup_content_process_shutdown_ack,…

… r=asajeffrey Restructure content process shutdown ack with threaded sender Recycling some work from #23909  ---  - [ ] `./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 ___    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23972)

bors-servo · 2019-08-17T22:51:09Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: asajeffrey
Pushing a8ae0e5 to master...

highfive assigned nox Aug 15, 2019

highfive added the S-awaiting-review label Aug 15, 2019

gterzian requested a review from asajeffrey Aug 15, 2019

highfive assigned asajeffrey and unassigned nox Aug 15, 2019

asajeffrey reviewed Aug 15, 2019

View changes

gterzian force-pushed the gterzian:cleanup_content_process_shutdown_ack branch 2 times, most recently from afc6480 to 4cd2730 Aug 15, 2019

gterzian requested a review from asajeffrey Aug 16, 2019

asajeffrey reviewed Aug 16, 2019

View changes

highfive added S-awaiting-merge and removed S-awaiting-review labels Aug 17, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 17, 2019

restructure content process shutdown ack with threaded sender, withou…

Loading status checks…

8482d2e

…t layout

gterzian force-pushed the gterzian:cleanup_content_process_shutdown_ack branch from 4cd2730 to 8482d2e Aug 17, 2019

highfive added S-awaiting-review and removed S-tests-failed labels Aug 17, 2019

highfive added S-awaiting-merge and removed S-awaiting-review labels Aug 17, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 17, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Aug 17, 2019

bors-servo merged commit 8482d2e into servo:master Aug 17, 2019
3 checks passed

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Aug 17, 2019

bors-servo mentioned this pull request Aug 17, 2019

[WIP]Introduce a shared ipc router and a ipc handle #23909

Closed

0 of 5 tasks complete

gterzian deleted the gterzian:cleanup_content_process_shutdown_ack branch Aug 18, 2019

servo / servo

Restructure content process shutdown ack with threaded sender #23972

Restructure content process shutdown ack with threaded sender #23972

gterzian commented Aug 15, 2019 •

edited by SimonSapin

highfive commented Aug 15, 2019

highfive commented Aug 15, 2019

gterzian commented Aug 15, 2019 •

edited

jdm commented Aug 15, 2019

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.

gterzian commented Aug 15, 2019

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.

asajeffrey commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

gterzian commented Aug 17, 2019

asajeffrey commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

gterzian commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

servo / servo

Join GitHub today

Restructure content process shutdown ack with threaded sender #23972

Restructure content process shutdown ack with threaded sender #23972

Conversation

gterzian commented Aug 15, 2019 • edited by SimonSapin

highfive commented Aug 15, 2019

highfive commented Aug 15, 2019

gterzian commented Aug 15, 2019 • edited

jdm commented Aug 15, 2019

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.

gterzian Aug 15, 2019 • edited

This comment has been minimized.

gterzian Aug 15, 2019 • edited

This comment has been minimized.

gterzian commented Aug 15, 2019

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.

asajeffrey commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

gterzian commented Aug 17, 2019

asajeffrey commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

gterzian commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

gterzian commented Aug 15, 2019 •

edited by SimonSapin

gterzian commented Aug 15, 2019 •

edited

gterzian Aug 15, 2019 •

edited

gterzian Aug 15, 2019 •

edited