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.

Sign up for GitHub

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

Jump to bottom

Implement Worker.prototype.terminate() #5846

Closed
wants to merge 7 commits into from
Closed

Implement Worker.prototype.terminate() #5846

wants to merge 7 commits into from

Conversation

jwilm
Copy link
Contributor

@jwilm jwilm commented Apr 26, 2015

Adds support for terminating DOM workers. ScriptMsg::Terminate was added to notify the WorkerGlobalScope/DedicatedWorkerGlobalScope to stop processing events and shut down. A closing flag was added to WorkerGlobalScope per the spec.

Additionally, a closing flag was added to dom::worker::Worker to ensure no further events are dispatched to the onmessage handler.

The change to the wpt WorkerTerminate.js is a patch for the broken Worker_terminate_event_queue test (asserts 10001 == 10000).

Resolves #4427

Review on Reviewable

@jwilm
Implement Worker.prototype.terminate()
5c895dd 
Adds support for terminating DOM workers. A new ScriptMsg::Terminate was
added to notify the WorkerGlobalScope/DedicatedWorkerGlobalScope to stop
processing events and shut down. A closing flag was added to
WorkerGlobalScope per the spec.

Additionally, a closing flag was added to dom::worker::Worker to ensure
no further events are dispatched to the onmessage handler.

The change to the wpt WorkerTerminate.js is a patch for the broken
Worker_terminate_event_queue test (asserts 10001 == 10000).

Resolves servo#4427
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 26, 2015
@highfive
Copy link

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon.

@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/4802

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@jwilm jwilm mentioned this pull request Apr 26, 2015
Open
jwilm
jwilm reviewed Apr 27, 2015
View reviewed changes
components/script/dom/worker.rs
@@ -100,6 +103,10 @@ impl Worker {
data: StructuredCloneData) {
let worker = address.to_temporary().root();

if worker.r().closing.get() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The spec specifies to empty the event/task queue associated with the worker as to not deliver further events to the onmessage handler. Without a synchronous way to affect the DedicatedWorkerGlobalScope, I resorted to these short-circuiting checks to prevent messages from being delivered. I keep thinking about this because it's kind of ugly. Does someone have any perspective on a cleaner way to achieve that behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gecko appears to differentiate between normal events in the worker thread's queue and what it calls a ControlRunnable. The run loop in their worker prioritizes the control events and handles at most one normal event on an iteration. We could implement something similar by adding a control channel and moving to a select model for the loop.

@Ms2ger
Copy link
Contributor

Ms2ger commented Apr 27, 2015

I commented on critic.

@Ms2ger Ms2ger added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 27, 2015
@jwilm
Copy link
Contributor Author

jwilm commented Apr 27, 2015

I left a couple of comments/questions in reply.

Do reviewers get notified of replies from critic, or should I ping from the issue?

@jdm
Copy link
Member

jdm commented Apr 27, 2015

The comments arrive in email form; no ping necessary.

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Apr 28, 2015
@jwilm
Copy link
Contributor Author

jwilm commented Apr 28, 2015

I fixed all of the issues on critic except escaping a long running JS handler. Interrupting JS execution in this case requires a call to JS_RequestInterrupt from Worker. This means that both Worker and DedicatedWorkerGlobalScope need a reference to the Runtime. However, we cannot Send Runtime or Cx in the current rust-mozjs, and so there's no way to schedule an interrupt. A higher level API in rust-mozjs that handles safe concurrent access to the runtime would make this possible.

@jwilm
Copy link
Contributor Author

jwilm commented Apr 30, 2015

The latest changes add support for killing a blocking script in the worker. Such capability requires a reference to the JSRuntime in Worker. I think this highlights the need for building out a sendable runtime in mozjs-rust. There's a couple of unsafe sections in both DedicatedWorkerGlobalScope and Worker to handle managing the mozjs interrupts. Again, we need more safe APIs from mozjs-rust.

It might be good to have a second channel for workers as well. I ended up needing to buffer ScriptMsgs outside of the receiver. This wouldn't even be a problem if there was a dedicated channel for control messages.

The code in tests/html/worker was for validation that the thread count was not increasing after spawning/terminating numerous workers. It would be nice to automate that, but for now there's test pages.

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels May 1, 2015
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels May 1, 2015
@jdm jdm added the S-needs-rebase There are merge conflict errors. label May 6, 2015
jwilm added 2 commits May 22, 2015 07:39
@jwilm
fixup! Implement Worker.prototype.terminate()
73593f5
@jwilm
Refactor shared code for global object unwrapping
0854e9e 
global_object_for_js_object and global_object_for_js_context shared a
lot of common code that has been moved into global_object_for_js_global.
@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 20, 2015

Please rebase on master.

@Ms2ger Ms2ger mentioned this pull request Jul 17, 2015
Closed
@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 17, 2015

Rebased as #6652

@Ms2ger Ms2ger closed this Jul 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Ms2ger Ms2ger

Labels
S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement Worker.terminate
5 participants
@jwilm @highfive @hoppipolla-critic-bot @Ms2ger @jdm