Skip to content

/ servo

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

Terminate timer scheduler thread during shutdown #16184

Merged
merged 1 commit into from Mar 30, 2017
Merged

Terminate timer scheduler thread during shutdown #16184

merged 1 commit into from Mar 30, 2017

Conversation

@ferjm
Copy link
Member

ferjm commented Mar 29, 2017 •
edited

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #16153

This change is Reviewable
@highfive
Copy link

highfive commented Mar 29, 2017

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/pipeline.rs, components/constellation/constellation.rs, components/constellation/timer_scheduler.rs
  • @fitzgen: components/script/timers.rs, components/script/dom/globalscope.rs, components/script/script_thread.rs, components/script/dom/window.rs, components/script_traits/lib.rs, components/script_traits/lib.rs
  • @KiChjang: components/script/timers.rs, components/script/dom/globalscope.rs, components/script/script_thread.rs, components/script/dom/window.rs, components/script_traits/lib.rs, components/script_traits/lib.rs
@highfive
Copy link

highfive commented Mar 29, 2017

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@ferjm
Copy link
Member Author

ferjm commented Mar 29, 2017

r? @jdm
@jdm
jdm requested changes Mar 30, 2017
View changes
Copy link
Member

jdm left a comment

This looks really good!
components/constellation/timer_scheduler.rs Outdated
let _ = req_sender.send(req);
if shutting_down {
break;
}
timeout_thread.unpark();

This comment has been minimized.

Sign in to view
@jdm

jdm Mar 30, 2017

Member

We should probably unpark the thread before breaking out of this loop.
@ferjm ferjm force-pushed the ferjm:issue-16153-terminate-time-scheduler-shutdown branch from 24eb03a to f004897 Mar 30, 2017
@ferjm ferjm requested a review from jdm Mar 30, 2017
@jdm
Copy link
Member

jdm commented Mar 30, 2017

Looks good. Squash and r=me!
@bors-servo: delegate+
@bors-servo
Copy link
Contributor

bors-servo commented Mar 30, 2017

✌️ @ferjm can now approve this pull request
@jdm
jdm approved these changes Mar 30, 2017
View changes
@ferjm
Terminate timer scheduler thread during shutdown
Loading status checks…
fb878f3
@ferjm ferjm force-pushed the ferjm:issue-16153-terminate-time-scheduler-shutdown branch from f004897 to fb878f3 Mar 30, 2017
@ferjm
Copy link
Member Author

ferjm commented Mar 30, 2017

Thank you Josh!

@bors-servo r=jdm
@bors-servo
Copy link
Contributor

bors-servo commented Mar 30, 2017

📌 Commit fb878f3 has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Mar 30, 2017

⌛ Testing commit fb878f3 with merge a3d9688...
bors-servo added a commit that referenced this pull request Mar 30, 2017
@bors-servo
Auto merge of #16184 - ferjm:issue-16153-terminate-time-scheduler-shu…
Loading status checks…
a3d9688 
…tdown, r=jdm

Terminate timer scheduler thread during shutdown

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #16153

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16184)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 30, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: jdm
Pushing a3d9688 to master...
@bors-servo bors-servo merged commit fb878f3 into servo:master Mar 30, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@ferjm ferjm deleted the ferjm:issue-16153-terminate-time-scheduler-shutdown branch Mar 30, 2017
@bors-servo bors-servo mentioned this pull request Mar 30, 2017
Closed
3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm

Assignees

@jdm jdm

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Terminate timer scheduler thread during shutdown
4 participants
@ferjm @highfive @jdm @bors-servo
You can’t perform that action at this time.