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

Introduce "per task source" cancellation #21126

Merged
merged 1 commit into from Jul 10, 2018
Merged

Introduce "per task source" cancellation #21126

merged 1 commit into from Jul 10, 2018
+127 −34

Conversation

@gterzian
Copy link
Member

gterzian commented Jul 5, 2018 •
edited by SimonSapin

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #21119 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable
@highfive
Copy link

highfive commented Jul 5, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/task_source/file_reading.rs, components/script/task_source/user_interaction.rs, components/script/dom/websocket.rs, components/script/fetch.rs, components/script/dom/globalscope.rs and 14 more
  • @KiChjang: components/script/task_source/file_reading.rs, components/script/task_source/user_interaction.rs, components/script/dom/websocket.rs, components/script/fetch.rs, components/script/dom/globalscope.rs and 14 more
@highfive
Copy link

highfive commented Jul 5, 2018

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@gterzian gterzian force-pushed the gterzian:tasksource_specific_cancelation branch 2 times, most recently from edd4f18 to ebaf7cd Jul 5, 2018
@gterzian gterzian changed the title [WIP] Introduce granular "per task source" cancellation [WIP] Introduce "per task source" cancellation Jul 5, 2018
@gterzian gterzian force-pushed the gterzian:tasksource_specific_cancelation branch 4 times, most recently from c277174 to e33b2ff Jul 5, 2018
@gterzian gterzian changed the title [WIP] Introduce "per task source" cancellation Introduce "per task source" cancellation Jul 5, 2018
@gterzian
Copy link
Member Author

gterzian commented Jul 5, 2018

This one is ready for review, although I'm wondering about the right testing strategy for this one...
@gterzian gterzian force-pushed the gterzian:tasksource_specific_cancelation branch 4 times, most recently from bd8c344 to 48adf6a Jul 5, 2018
@gterzian
Copy link
Member Author

gterzian commented Jul 5, 2018 •
edited

For clarity this isn't suppose to 'change' anything about the behavior of the system, it only lays the groundwork for later changes, for example in #21114, which will require using a dedicated task source for EventSource and then only cancelling the networking tasks as part of document.abort, and letting the event source tasks run(allowing us to fail the connection after having cancelled the fetches).

So perhaps a good way to test is 'making sure everything still works as before', and then it is the follow up work that will include tests specifically asserting that certain tasks are cancelled while others still run?
@gterzian gterzian force-pushed the gterzian:tasksource_specific_cancelation branch 6 times, most recently from 9f20d1e to 8799aa0 Jul 5, 2018
@jdm
jdm requested changes Jul 10, 2018
View changes
components/script/task_source/mod.rs Outdated
// Retuns a vec of variants of TaskSourceName.
// Note: When adding a variant, update the vec.
pub fn all() -> Vec<TaskSourceName> {
vec![

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Jul 10, 2018

Member

We might as well make use of https://crates.io/crates/enum-iterator rather than keep track of this manually.
components/script/task_source/mod.rs Outdated
@@ -22,10 +51,13 @@ pub trait TaskSource {
where
T: TaskOnce + 'static;

fn choose_canceller(&self, global: &GlobalScope) -> TaskCanceller;

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Jul 10, 2018

Member

It might make sense to use an associated constant instead, rather than having similar implementations for each task source.
components/script/layout_image.rs
@@ -62,7 +63,7 @@ pub fn fetch_image_for_layout(url: ServoUrl,
let listener = NetworkListener {
context: context,
task_source: window.networking_task_source(),
canceller: Some(window.task_canceller()),
canceller: Some(window.task_canceller(TaskSourceName::Networking)),

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Jul 10, 2018

Member

As a followup, we should do something to avoid all of the duplication of calling foo_task_source and then getting a canceller where it's easy to end up with one for a different source. Possibly the foo_task_source APIs could return both a source and a canceller at the same time.

This comment has been minimized.

Sign in to view
Copy link
@gterzian

gterzian Jul 10, 2018

Author Member

I've filed #21154
@gterzian gterzian mentioned this pull request Jul 10, 2018
Closed
@gterzian gterzian force-pushed the gterzian:tasksource_specific_cancelation branch from 044387f to 731fdee Jul 10, 2018
@gterzian
Copy link
Member Author

gterzian commented Jul 10, 2018

@jdm Thanks for the pointers, comments have been addressed...
@gterzian
introduce "per task source" ignoring of tasks
Loading status checks…
671627e
@gterzian gterzian force-pushed the gterzian:tasksource_specific_cancelation branch from 731fdee to 671627e Jul 10, 2018
@jdm
Copy link
Member

jdm commented Jul 10, 2018

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Jul 10, 2018

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

bors-servo commented Jul 10, 2018

⌛ Testing commit 671627e with merge 8988d67...
bors-servo added a commit that referenced this pull request Jul 10, 2018
@bors-servo
Auto merge of #21126 - gterzian:tasksource_specific_cancelation, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
8988d67 
Introduce "per task source" cancellation

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #21119 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/21126)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 10, 2018

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 8988d67 to master...
@bors-servo bors-servo merged commit 671627e into servo:master Jul 10, 2018
3 of 4 checks passed
3 of 4 checks passed
Taskcluster (pull_request) TaskGroup: failure
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@gterzian gterzian deleted the gterzian:tasksource_specific_cancelation branch Jul 10, 2018
@gterzian gterzian mentioned this pull request Jul 13, 2018
Merged
0 of 5 tasks complete
bors-servo added a commit that referenced this pull request Jul 13, 2018
@bors-servo
Auto merge of #21170 - gterzian:use_remote_event_task_source_for_even…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
fb6eea3 
…tsource, r=jdm

Use remote-event task source in EventSource

<!-- Please describe your changes on the following line: -->
Follow up on #21126, this again 'changes nothing', but will be useful for #21111 in the context of #21114

Trying to break up work into smaller PR's ;)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #21112 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/21170)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Jul 13, 2018
@bors-servo
Auto merge of #21170 - gterzian:use_remote_event_task_source_for_even…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
9304578 
…tsource, r=jdm

Use remote-event task source in EventSource

<!-- Please describe your changes on the following line: -->
Follow up on #21126, this again 'changes nothing', but will be useful for #21114 in the context of #21111.

Trying to break up work into smaller PR's ;)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #21112 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/21170)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Jul 14, 2018
@bors-servo
Auto merge of #21170 - gterzian:use_remote_event_task_source_for_even…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
8eb2a53 
…tsource, r=jdm

Use remote-event task source in EventSource

<!-- Please describe your changes on the following line: -->
Follow up on #21126, this again 'changes nothing', but will be useful for #21114 in the context of #21111.

Trying to break up work into smaller PR's ;)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #21112 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/21170)
<!-- Reviewable:end -->
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.

TaskSource-specific TaskCanceller
4 participants
@gterzian @highfive @jdm @bors-servo
You can’t perform that action at this time.