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.

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

Fetch cancellation #19274

Merged
merged 6 commits into from Nov 21, 2017
Merged

Fetch cancellation #19274

merged 6 commits into from Nov 21, 2017

Conversation

@Manishearth
Copy link
Member

Manishearth commented Nov 17, 2017

This PR implements cancellation for fetch, and uses it for XHR. This means that fetch clients can now send a message to the fetch task asking for the network request to be aborted.

Previously, clients like XHR had abort functionality but would implement it by simply ignoring future messages from the network task; and would not actually cancel the network fetch.


This change is Reviewable

@highfive
Copy link

highfive commented Nov 17, 2017

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/network_listener.rs
  • @cbrewster: components/constellation/network_listener.rs
  • @paulrouget: components/constellation/network_listener.rs
  • @fitzgen: components/script/dom/xmlhttprequest.rs, components/script/document_loader.rs, components/script/dom/eventsource.rs, components/script/fetch.rs
  • @KiChjang: components/net/fetch/methods.rs, components/net/resource_thread.rs, components/net/http_loader.rs, components/script/dom/xmlhttprequest.rs, components/net_traits/lib.rs and 4 more
@highfive
Copy link

highfive commented Nov 17, 2017

warning Warning warning

  • These commits modify net and script code, but no tests are modified. Please consider adding a test!
@Manishearth Manishearth force-pushed the Manishearth:xhr-cancel branch from ef7088a to d82315a Nov 17, 2017
@Manishearth
Copy link
Member Author

Manishearth commented Nov 17, 2017

r? @jdm

@highfive highfive assigned jdm and unassigned emilio Nov 17, 2017
@Manishearth
Copy link
Member Author

Manishearth commented Nov 17, 2017

Still need to test this

@Manishearth
Copy link
Member Author

Manishearth commented Nov 17, 2017

Tested manually, it works.

Testing methodology: Listen with nc -l port, have page set up XHR to listener with a button that aborts it, send back an HTTP response with content-length: largenumber, keep typing really long lines as responses (need to exceed the default chunking of hyper, I was sending responses that were just 10k "a"s). XHR should accept these responses just fine, but on calling abort the next chunk sent over should terminate netcat by closing the connection.

@@ -1109,6 +1110,11 @@ fn http_network_fetch(request: &Request,
}

loop {
if cancellation_listener.lock().unwrap().cancelled() {
*res_body.lock().unwrap() = ResponseBody::Done(vec![]);

This comment has been minimized.

@gterzian

gterzian Nov 18, 2017

Member

In the context of #18676 I think that at this point the response would have been cached already, and such a cancellation would result in any subsequent request for the same url getting the empty cancelled response from the cache.

One way to solve this would be for the cache to disregard any cached responses that would have been cancelled. Checking for an empty response body might be one way to do so, perhaps a clearer way would be to use ResponseBody::Empty here instead, or even introduce a new ResponseBody::Cancelled?

This comment has been minimized.

@gterzian

gterzian Nov 18, 2017

Member

Having had a look at the spec, the following can be considered:

  1. At step 5 above, if at that point the fetch has already been aborted/terminated, you could return a network error.
  2. If the fetch is terminated/aborted while the fetch worker is already performing step 12, in addition to what is done here and instead of what I suggested earlier, you could set a new aborted flag on the response. The cache could then also ignore any such aborted responses that would be held in it, when asked to construct a response. The spec mentions such a flag, see https://fetch.spec.whatwg.org/#concept-response-aborted, and it doesn't look like it has been implemented yet.

This comment has been minimized.

@jdm

jdm Nov 20, 2017

Member

Let's add the flag to the response and set that here, and use the network error for prior to this point like gterzian suggested.

@Manishearth Manishearth force-pushed the Manishearth:xhr-cancel branch from d82315a to 5b2b68d Nov 18, 2017
@Manishearth
Copy link
Member Author

Manishearth commented Nov 18, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Nov 18, 2017

Trying commit 5b2b68d with merge a82585d...

bors-servo added a commit that referenced this pull request Nov 18, 2017
Fetch cancellation

This PR implements cancellation for fetch, and uses it for XHR. This means that fetch clients can now send a message to the fetch task asking for the network request to be aborted.

Previously, clients like XHR had abort functionality but would implement it by simply ignoring future messages from the network task; and would not actually cancel the network fetch.

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

bors-servo commented Nov 18, 2017

Copy link
Member

jdm left a comment

Nice and readable!

if let Some(ref cancel_chan) = self.cancel_chan {
if self.cancelled {
true
} else if let Ok(_) = cancel_chan.try_recv() {

This comment has been minimized.

@jdm

jdm Nov 20, 2017

Member

nit: is_ok

@@ -1109,6 +1110,11 @@ fn http_network_fetch(request: &Request,
}

loop {
if cancellation_listener.lock().unwrap().cancelled() {
*res_body.lock().unwrap() = ResponseBody::Done(vec![]);

This comment has been minimized.

@jdm

jdm Nov 20, 2017

Member

Let's add the flag to the response and set that here, and use the network error for prior to this point like gterzian suggested.

@Manishearth Manishearth force-pushed the Manishearth:xhr-cancel branch from 5b2b68d to 14115ce Nov 21, 2017
@Manishearth Manishearth force-pushed the Manishearth:xhr-cancel branch from 14115ce to 6f59b15 Nov 21, 2017
@Manishearth
Copy link
Member Author

Manishearth commented Nov 21, 2017

Updated. Nit fixed and rolled into previous commit, added new commit for aborted flag.

@jdm
Copy link
Member

jdm commented Nov 21, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Nov 21, 2017

📌 Commit 6f59b15 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Nov 21, 2017

Testing commit 6f59b15 with merge 00b3612...

bors-servo added a commit that referenced this pull request Nov 21, 2017
Fetch cancellation

This PR implements cancellation for fetch, and uses it for XHR. This means that fetch clients can now send a message to the fetch task asking for the network request to be aborted.

Previously, clients like XHR had abort functionality but would implement it by simply ignoring future messages from the network task; and would not actually cancel the network fetch.

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

bors-servo commented Nov 21, 2017

@bors-servo bors-servo merged commit 6f59b15 into servo:master Nov 21, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@jdm jdm mentioned this pull request Nov 21, 2017
@gterzian gterzian mentioned this pull request Nov 23, 2017
0 of 5 tasks complete
bors-servo added a commit that referenced this pull request Dec 4, 2017
…r=<try>

Ignore aborted responses in caching

<!-- Please describe your changes on the following line: -->
@jdm @KiChjang @Manishearth Follow up on #18676 and #19274 to ignore aborted responses in caching.

I also found out the cache shouldn't return any response whose body is still in `ResponseBody::Receiving` mode, because that fails the assertion at https://github.com/servo/servo/blob/master/components/net/fetch/methods.rs#L438(we might want to add a channel as pat of the cached response later on to deal with this case). I only found out now because I needed the response from the server to trickle in so that it could be cached and aborted.

I copied the `http-cache.py` server from the wpt folder, and added a 'trickle' option, which is necessary to actually have a failing test with a cached but aborted request, it's now passing.

I also remove one unused import that slippled through previously.

---
<!-- 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 #__ (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/19350)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Dec 15, 2017
…r=<try>

Ignore aborted responses in caching

<!-- Please describe your changes on the following line: -->
@jdm @KiChjang @Manishearth Follow up on #18676 and #19274 to ignore aborted responses in caching.

I also found out the cache shouldn't return any response whose body is still in `ResponseBody::Receiving` mode, because that fails the assertion at https://github.com/servo/servo/blob/master/components/net/fetch/methods.rs#L438(we might want to add a channel as pat of the cached response later on to deal with this case). I only found out now because I needed the response from the server to trickle in so that it could be cached and aborted.

I copied the `http-cache.py` server from the wpt folder, and added a 'trickle' option, which is necessary to actually have a failing test with a cached but aborted request, it's now passing.

I also remove one unused import that slippled through previously.

---
<!-- 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 #__ (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/19350)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Dec 16, 2017
…r=<try>

Ignore aborted responses in caching

<!-- Please describe your changes on the following line: -->
@jdm @KiChjang @Manishearth Follow up on #18676 and #19274 to ignore aborted responses in caching.

I also found out the cache shouldn't return any response whose body is still in `ResponseBody::Receiving` mode, because that fails the assertion at https://github.com/servo/servo/blob/master/components/net/fetch/methods.rs#L438(we might want to add a channel as pat of the cached response later on to deal with this case). I only found out now because I needed the response from the server to trickle in so that it could be cached and aborted.

I copied the `http-cache.py` server from the wpt folder, and added a 'trickle' option, which is necessary to actually have a failing test with a cached but aborted request, it's now passing.

I also remove one unused import that slippled through previously.

---
<!-- 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 #__ (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/19350)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Jan 23, 2018
…r=<try>

Ignore aborted responses in caching

<!-- Please describe your changes on the following line: -->
@jdm @KiChjang @Manishearth Follow up on #18676 and #19274 to ignore aborted responses in caching.

I also found out the cache shouldn't return any response whose body is still in `ResponseBody::Receiving` mode, because that fails the assertion at https://github.com/servo/servo/blob/master/components/net/fetch/methods.rs#L438(we might want to add a channel as pat of the cached response later on to deal with this case). I only found out now because I needed the response from the server to trickle in so that it could be cached and aborted.

I copied the `http-cache.py` server from the wpt folder, and added a 'trickle' option, which is necessary to actually have a failing test with a cached but aborted request, it's now passing.

I also remove one unused import that slippled through previously.

---
<!-- 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 #__ (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/19350)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Jan 23, 2018
…r=<try>

Ignore aborted responses in caching

<!-- Please describe your changes on the following line: -->
@jdm @KiChjang @Manishearth Follow up on #18676 and #19274 to ignore aborted responses in caching.

I also found out the cache shouldn't return any response whose body is still in `ResponseBody::Receiving` mode, because that fails the assertion at https://github.com/servo/servo/blob/master/components/net/fetch/methods.rs#L438(we might want to add a channel as pat of the cached response later on to deal with this case). I only found out now because I needed the response from the server to trickle in so that it could be cached and aborted.

I copied the `http-cache.py` server from the wpt folder, and added a 'trickle' option, which is necessary to actually have a failing test with a cached but aborted request, it's now passing.

I also remove one unused import that slippled through previously.

---
<!-- 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 #__ (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/19350)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Jan 23, 2018
…r=<try>

Ignore aborted responses in caching

<!-- Please describe your changes on the following line: -->
@jdm @KiChjang @Manishearth Follow up on #18676 and #19274 to ignore aborted responses in caching.

I also found out the cache shouldn't return any response whose body is still in `ResponseBody::Receiving` mode, because that fails the assertion at https://github.com/servo/servo/blob/master/components/net/fetch/methods.rs#L438(we might want to add a channel as pat of the cached response later on to deal with this case). I only found out now because I needed the response from the server to trickle in so that it could be cached and aborted.

I copied the `http-cache.py` server from the wpt folder, and added a 'trickle' option, which is necessary to actually have a failing test with a cached but aborted request, it's now passing.

I also remove one unused import that slippled through previously.

---
<!-- 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 #__ (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/19350)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Jan 23, 2018
…r=jdm

Ignore aborted responses in caching

<!-- Please describe your changes on the following line: -->
@jdm @KiChjang @Manishearth Follow up on #18676 and #19274 to ignore aborted responses in caching.

I also found out the cache shouldn't return any response whose body is still in `ResponseBody::Receiving` mode, because that fails the assertion at https://github.com/servo/servo/blob/master/components/net/fetch/methods.rs#L438(we might want to add a channel as pat of the cached response later on to deal with this case). I only found out now because I needed the response from the server to trickle in so that it could be cached and aborted.

I copied the `http-cache.py` server from the wpt folder, and added a 'trickle' option, which is necessary to actually have a failing test with a cached but aborted request, it's now passing.

I also remove one unused import that slippled through previously.

---
<!-- 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 #__ (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/19350)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Jan 23, 2018
…r=jdm

Ignore aborted responses in caching

<!-- Please describe your changes on the following line: -->
@jdm @KiChjang @Manishearth Follow up on #18676 and #19274 to ignore aborted responses in caching.

I also found out the cache shouldn't return any response whose body is still in `ResponseBody::Receiving` mode, because that fails the assertion at https://github.com/servo/servo/blob/master/components/net/fetch/methods.rs#L438(we might want to add a channel as pat of the cached response later on to deal with this case). I only found out now because I needed the response from the server to trickle in so that it could be cached and aborted.

I copied the `http-cache.py` server from the wpt folder, and added a 'trickle' option, which is necessary to actually have a failing test with a cached but aborted request, it's now passing.

I also remove one unused import that slippled through previously.

---
<!-- 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 #__ (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/19350)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Jan 23, 2018
…r=jdm

Ignore aborted responses in caching

<!-- Please describe your changes on the following line: -->
@jdm @KiChjang @Manishearth Follow up on #18676 and #19274 to ignore aborted responses in caching.

I also found out the cache shouldn't return any response whose body is still in `ResponseBody::Receiving` mode, because that fails the assertion at https://github.com/servo/servo/blob/master/components/net/fetch/methods.rs#L438(we might want to add a channel as pat of the cached response later on to deal with this case). I only found out now because I needed the response from the server to trickle in so that it could be cached and aborted.

I copied the `http-cache.py` server from the wpt folder, and added a 'trickle' option, which is necessary to actually have a failing test with a cached but aborted request, it's now passing.

I also remove one unused import that slippled through previously.

---
<!-- 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 #__ (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/19350)
<!-- Reviewable:end -->
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jan 24, 2018
…ian:ignore_aborted_responses_in_caching); r=jdm

<!-- Please describe your changes on the following line: -->
@jdm @KiChjang @Manishearth Follow up on servo/servo#18676 and servo/servo#19274 to ignore aborted responses in caching.

I also found out the cache shouldn't return any response whose body is still in `ResponseBody::Receiving` mode, because that fails the assertion at https://github.com/servo/servo/blob/master/components/net/fetch/methods.rs#L438(we might want to add a channel as pat of the cached response later on to deal with this case). I only found out now because I needed the response from the server to trickle in so that it could be cached and aborted.

I copied the `http-cache.py` server from the wpt folder, and added a 'trickle' option, which is necessary to actually have a failing test with a cached but aborted request, it's now passing.

I also remove one unused import that slippled through previously.

---
<!-- 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 #__ (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. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 4307b6e67b0cb35e2afc46ba0b64e7bc5bde1bdf

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : 4631dd57c5f691119b287f2aa04ee97fedfdf406
@Manishearth Manishearth deleted the Manishearth:xhr-cancel branch Jul 11, 2019
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 2, 2019
…ian:ignore_aborted_responses_in_caching); r=jdm

<!-- Please describe your changes on the following line: -->
jdm KiChjang Manishearth Follow up on servo/servo#18676 and servo/servo#19274 to ignore aborted responses in caching.

I also found out the cache shouldn't return any response whose body is still in `ResponseBody::Receiving` mode, because that fails the assertion at https://github.com/servo/servo/blob/master/components/net/fetch/methods.rs#L438(we might want to add a channel as pat of the cached response later on to deal with this case). I only found out now because I needed the response from the server to trickle in so that it could be cached and aborted.

I copied the `http-cache.py` server from the wpt folder, and added a 'trickle' option, which is necessary to actually have a failing test with a cached but aborted request, it's now passing.

I also remove one unused import that slippled through previously.

---
<!-- 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 #__ (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. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 4307b6e67b0cb35e2afc46ba0b64e7bc5bde1bdf

UltraBlame original commit: e11d6ca7abe7416d5cead0c104b77a9b106f6363
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 2, 2019
…ian:ignore_aborted_responses_in_caching); r=jdm

<!-- Please describe your changes on the following line: -->
jdm KiChjang Manishearth Follow up on servo/servo#18676 and servo/servo#19274 to ignore aborted responses in caching.

I also found out the cache shouldn't return any response whose body is still in `ResponseBody::Receiving` mode, because that fails the assertion at https://github.com/servo/servo/blob/master/components/net/fetch/methods.rs#L438(we might want to add a channel as pat of the cached response later on to deal with this case). I only found out now because I needed the response from the server to trickle in so that it could be cached and aborted.

I copied the `http-cache.py` server from the wpt folder, and added a 'trickle' option, which is necessary to actually have a failing test with a cached but aborted request, it's now passing.

I also remove one unused import that slippled through previously.

---
<!-- 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 #__ (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. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 4307b6e67b0cb35e2afc46ba0b64e7bc5bde1bdf

UltraBlame original commit: e11d6ca7abe7416d5cead0c104b77a9b106f6363
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 2, 2019
…ian:ignore_aborted_responses_in_caching); r=jdm

<!-- Please describe your changes on the following line: -->
jdm KiChjang Manishearth Follow up on servo/servo#18676 and servo/servo#19274 to ignore aborted responses in caching.

I also found out the cache shouldn't return any response whose body is still in `ResponseBody::Receiving` mode, because that fails the assertion at https://github.com/servo/servo/blob/master/components/net/fetch/methods.rs#L438(we might want to add a channel as pat of the cached response later on to deal with this case). I only found out now because I needed the response from the server to trickle in so that it could be cached and aborted.

I copied the `http-cache.py` server from the wpt folder, and added a 'trickle' option, which is necessary to actually have a failing test with a cached but aborted request, it's now passing.

I also remove one unused import that slippled through previously.

---
<!-- 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 #__ (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. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 4307b6e67b0cb35e2afc46ba0b64e7bc5bde1bdf

UltraBlame original commit: e11d6ca7abe7416d5cead0c104b77a9b106f6363
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.