Ignore aborted responses in caching

[gterzian]

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.

---

• ./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]

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

[highfive]

Heads up! This PR modifies the following files:

• @KiChjang: components/net/fetch/methods.rs, components/net/http_cache.rs, components/net_traits/response.rs, components/net_traits/response.rs

[jdm]

If the only change necessary was adding a trickle option, we can get that automatically with the existing script: http://wptserve.readthedocs.io/en/latest/pipes.html#trickle

[gterzian]

thanks for the pointer.

I tried applying the trickle option when making the XHR call, however it appears that such 'pipes' are only applied to static files, not python scripts(and the cache logic in the script is still necessary in addition to the trickle). The docs don't mention this, however in the code it really seems that wrap_pipeline is only applied to the response of the FileHandler(and therefore not to response of the PythonScriptHandler), see https://github.com/w3c/web-platform-tests/blob/master/tools/wptserve/wptserve/handlers.py#L150.
I opened a PR to update the docs by the way, in case this is correct(it would have saved me some debugging time to have a note present) web-platform-tests/wpt#8415

[gterzian]

Ok, an extra commit to handle ResponseBody::Receiving. That one is perhaps a bit of a hack, please take a look.

To allow the cache to construct a response that hasn't finished receiving it's body yet, the cache simply has to work with the "done channel", in a similar way to the fetch worker thread.

The problem I encountered related to aborting a fetch. Consider the following example:

1. Fetch 1 does a network fetch for resource A.
2. The response from Fetch 1, with a body of ResponseBody::Receiving, is cached.
3. While Fetch 1 is still receiving the body, Fetch 2 starts, for the same resource A.

At this point, if we allow the cache to construct a response with a body of ResponseBody::Receiving, we also need to set the done channel to Some, otherwise fetch/method expects a body in either of ResponseBody::Doneorof ResponseBody::Empty (a 'sync' response).

4. Fetch 2 gets a cached response, and awaits the body.
5. Fetch 1, before having received the full body, is aborted.
6. Fetch 2 receives a response with an empty ResponseBody::Done. This doesn't reflect the content of the resource that Fetch 2 asked for. There is no way at this point to tell Fetch 2 to go to the network after all.

To handle the problem of 6, I added a loop in http_loader.rs, that awaits for either Data::Done or Data::Cancelled. In the case of Done, the full response is returned, this is the equivalent of having a full response in the cache to begin with. In the case of Cancel, the response is set back to None, and the network fetch takes over.

I amended the test in http-cache-xrh.js to reflect this. it now tests for the above scenario, and was failing until I added that new loop.

Please let me know what you think.

[Manishearth]

I'm not clear why we need more uses of done_chan here?

[gterzian]

@Manishearth One could say it's pretty much because of the logic in wait_for_response.

Here is the situation:

1. A network fetch will, unless the response is loaded immediatly, result in a response whose body is still receiving bytes, and a done_chan used to communicate when that is finished. The main fetch expects to receive such a done_chan whenever the body of a response is still receiving.
2. If we want to be able to construct cached responses, from a cached resource whose body is still receiving bytes(from within another fetch thread), the cache will need to return a done_chan as well, otherwise the assertion in wait_for_response will fail.
3. Obviously we don't need to follow the logic under 2, we can also simply have 2 result in a cache miss and a second network fetch for the same resource.
4. There is another problem that arises if the "original" fetch, the one that is fetching the body for it's "own" response, as well as for the cached resource, is aborted. The problem is that for the target of the original fetch, the one that aborted it, it's normal for the response to be empty, or a network error. However that isn't the case for other targets who are still awaiting the body of that original fetch, but are doing so in the context of a cached response(all the other fetches that might have started for the same resources, while the original one hadn;t finished receiving, and while the response from the original one was already cached). The practical problem in this is that from within wait_for_response, or the point where it is called in fetch/method, it isn't obvious how to go back and to a network fetch to complete the response body.

The somewhat hacky solution I have found so far for the problem under 4, is to have this loop receive on the done_chan set by the cache, and in the case of an aborted fetch, discard the cached response and do a network fetch after all.

[gterzian]

In the context of this PR, the easy solution is to disregard the second commit, that way we have a cache that only deals in responses that have been fully loaded, and it will simply filter out those that have been aborted.

I do think the second commit highlights an interesting problem, perhaps even something that, if we get it right, could help clarify the spec.

The spec makes it obvious that "Typically response’s body’s stream is still being enqueued to after returning", but doesn't quite tell us how this fits in with caching. In fact caching is mostly referred to as a fully sync process in the spec(it's not clear if the cache can return a response whose body stream is still being enqueued to from another fetch).

The question of a fetch being aborted makes this even more interesting. What to do with a response from the cache, whose body is still being enqueued to from another fetch, when that other fetch is being aborted?

It's easy to filter out cached responses that have been already aborted when the subsequent fetch asks for them, it's harder to deal with the "async" situation of the cached response still waiting on the body being fetched by another fetch, who then is aborted...

I have a feeling that the relationship between caching, fetching, and aborting could use some additional norms.

[gterzian]

Have you had time to form thoughts on this one? Looking forward to those...

Regarding the 'trickle' inside the python cache script, I've opened a PR to allow applying pipes to python scripts too. web-platform-tests/wpt#8541, so in some way this PR is blocked by that one.

In the meantime, could we perhaps try a test run on this one? It's a bit too slow on my machine, and I'm wondering how the current changes fare against the full suite...

[bors-servo]

⌛ Testing commit 993e2f5 with merge ce05a29...

[bors-servo]

💔 Test failed - linux-rel-css

[CYBAI]

From filtered-wpt-errorsummary.log,

{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/_mozilla/webgl/conformance-1.0.3/conformance/more/functions/readPixelsBadArgs.html", "line": 190256, "action": "test_result", "expected": "OK"}

[jdm]

@bors-servo retry

• #19846

[bors-servo]

⌛ Testing commit 993e2f5 with merge 6d1c958...

[bors-servo]

💔 Test failed - linux-rel-css

[jdm]

@bors-servo retry

• #19033

[bors-servo]

⌛ Testing commit 993e2f5 with merge 4307b6e...

[bors-servo]

☀️ 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 4307b6e to master...

[gterzian]

thanks! I'll update the wpt code and remove the then unnecessary tests/mozilla/resources/http-cache-trickle.py script in a separate PR...