Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23710059:
Release the cache entry on deferred redirect. (Closed)
|
Created:
7 years, 3 months ago by davidben Modified:
7 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRelease the cache entry on deferred redirect.
If a redirect is deferred in the warm cache case, the HTTP transaction
currently doesn't release the cache entry until after the redirect continues.
This prevents other requests from accessing the cache entry.
Reuse the DoneReading() signal for the URLRequestJob to notify the transaction
that the response body is to be discarded. This matches the redirect-specific
logic in HttpCache::Transaction which does not cache redirect bodies.
In addition, now that this signal exists at a higher level, remove that logic
in HttpCache::Transaction. It is now the caller's job to decide which response
bodies are and aren't truncated away.
Fixup and add new tests for this behavior.
BUG=292879
TEST=HttpCache.CachedRedirect,
HttpCache.DoneReading,
URLRequestJob.RedirectTransactionNotifiedWhenDone
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224161
Patch Set 1 #
Total comments: 13
Patch Set 2 : Disable new tests on Chrome Frame #
Total comments: 20
Patch Set 3 : #
Total comments: 12
Patch Set 4 : #
Messages
Total messages: 18 (0 generated)
I wasn't sure whether it would be better to add a new method to HttpTransaction or to reuse DoneReading. I think it fits fairly well, even though I have to remove the DCHECKs that assume it's not called before Read. As I understand, DoneReading is so the cache knows that the request wasn't canceled if a filter sees some gzip EOF header or so and stops early. We're already discarding redirect responses in HttpCache::Transaction, so this just makes redirects a "filter" which discards the entire body (and thus never calls Read).
I think this looks pretty good, though want to double-check for any unintended consequences. https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_u... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_u... net/url_request/url_request_unittest.cc:4206: nit: Remove extra blank line. https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_u... net/url_request/url_request_unittest.cc:4207: TEST_F(URLRequestTestHTTP, CachedRedirect) { Maybe a second test where the redirect has a non-empty body? https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_u... net/url_request/url_request_unittest.cc:4340: TestDelegate d_defer, d; nit: Define one variable per line. https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_u... net/url_request/url_request_unittest.cc:4340: TestDelegate d_defer, d; nit: While this file name TestDelegates "d" a lot (In violation of Google style), I think "d_defer" is going a bit too far. Suggest just writing it out as "delegate". https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_u... net/url_request/url_request_unittest.cc:4345: URLRequest req_defer(test_url, &d_defer, &default_context_); optional: Suggest "deferred_req" and "deferring_delegate". Think it makes the relationship clearer. https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_u... net/url_request/url_request_unittest.cc:4359: EXPECT_EQ(URLRequestStatus::SUCCESS, req.status().status()); May want to mention something about the "deferred" redirect never being result - think that some contradicts the word "deferred". Or could rename it to "indefinitely_defered_redirect" or something - open to better ideas. Maybe "paused_"? "Halted" implies too much of a full stop...
+erikwright for chrome_frame/test/net/fake_external_tab.cc. I don't know anything about Chrome Frame, but the new tests check URLRequest::was_cached, and every other URLRequest test which checks that was disabled too, so I assume there's something funny about the CF cache? https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_u... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_u... net/url_request/url_request_unittest.cc:4206: On 2013/09/16 20:41:08, mmenke wrote: > nit: Remove extra blank line. Done. https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_u... net/url_request/url_request_unittest.cc:4207: TEST_F(URLRequestTestHTTP, CachedRedirect) { On 2013/09/16 20:41:08, mmenke wrote: > Maybe a second test where the redirect has a non-empty body? Are you sure it doesn't already have a non-empty body? redirect-test.html says "hello" in it. https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_u... net/url_request/url_request_unittest.cc:4340: TestDelegate d_defer, d; On 2013/09/16 20:41:08, mmenke wrote: > nit: Define one variable per line. Done. https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_u... net/url_request/url_request_unittest.cc:4340: TestDelegate d_defer, d; On 2013/09/16 20:41:08, mmenke wrote: > nit: While this file name TestDelegates "d" a lot (In violation of Google > style), I think "d_defer" is going a bit too far. Suggest just writing it out > as "delegate". Done. https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_u... net/url_request/url_request_unittest.cc:4345: URLRequest req_defer(test_url, &d_defer, &default_context_); On 2013/09/16 20:41:08, mmenke wrote: > optional: Suggest "deferred_req" and "deferring_delegate". Think it makes the > relationship clearer. Done. https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_u... net/url_request/url_request_unittest.cc:4359: EXPECT_EQ(URLRequestStatus::SUCCESS, req.status().status()); On 2013/09/16 20:41:08, mmenke wrote: > May want to mention something about the "deferred" redirect never being result - > think that some contradicts the word "deferred". Or could rename it to > "indefinitely_defered_redirect" or something - open to better ideas. Maybe > "paused_"? "Halted" implies too much of a full stop... Went with paused.
https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_u... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_u... net/url_request/url_request_unittest.cc:4207: TEST_F(URLRequestTestHTTP, CachedRedirect) { On 2013/09/16 22:00:00, David Benjamin wrote: > On 2013/09/16 20:41:08, mmenke wrote: > > Maybe a second test where the redirect has a non-empty body? > > Are you sure it doesn't already have a non-empty body? redirect-test.html says > "hello" in it. Looks like you're right. I saw the mock-headers, which don't have a Content-Length line, and just assumed there was no body. Our tests violate the HTTP/1.1 spec (Though they'd still be in violation with a 0-length body, too). :( Server should claim HTTP/1.0 or be sending a Content-Length header, dagnabit!
On 2013/09/16 22:08:59, mmenke wrote: > https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_u... > File net/url_request/url_request_unittest.cc (right): > > https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_u... > net/url_request/url_request_unittest.cc:4207: TEST_F(URLRequestTestHTTP, > CachedRedirect) { > On 2013/09/16 22:00:00, David Benjamin wrote: > > On 2013/09/16 20:41:08, mmenke wrote: > > > Maybe a second test where the redirect has a non-empty body? > > > > Are you sure it doesn't already have a non-empty body? redirect-test.html says > > "hello" in it. > > Looks like you're right. I saw the mock-headers, which don't have a > Content-Length line, and just assumed there was no body. Our tests violate the > HTTP/1.1 spec (Though they'd still be in violation with a 0-length body, too). > :( Server should claim HTTP/1.0 or be sending a Content-Length header, > dagnabit! rvargas: I think you added DoneReading, right? Does this change look reasonable?
chrome_frame LGTM. Chrome Frame doesn't use the Chrome network stack, so it doesn't use the Chrome HTTP cache.
https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_trans... File net/http/http_cache_transaction.cc (left): https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_trans... net/http/http_cache_transaction.cc:453: DCHECK_NE(mode_, UPDATE); Is this line failing? UPDATE means that the caller is attempting manual revalidation, and the transaction is not even able to read the data, just the headers. Being here for UPDATE means that something is weird and most likely someone is doing something unexpected. Maybe it triggers because line 1346 is gone... https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_trans... net/http/http_cache_transaction.cc:1346: DoneWritingToEntry(true); Even though it is redundant in the sense that the job _should_ call DoneReading, it is better to keep this logic here so that we don't depend on the job issuing that call. https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_unitt... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:5326: TEST(HttpCache, CachedRedirect) { I think is better to write a new test closer to the actual behavior change: now calling DoneReading() before Read() should release a transaction. Something close to HttpCache.FilterCompletion but without deleting the first transaction. https://codereview.chromium.org/23710059/diff/11001/net/http/http_transaction.h File net/http/http_transaction.h (right): https://codereview.chromium.org/23710059/diff/11001/net/http/http_transaction... net/http/http_transaction.h:112: // This is used to inform the cache that the response was not canceled nit: remove this sentence... or replace "cache" with transaction and "response" with something else :(. (the response is not canceled, it is the transaction). https://codereview.chromium.org/23710059/diff/11001/net/url_request/url_reque... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/23710059/diff/11001/net/url_request/url_reque... net/url_request/url_request_job.cc:323: // both so the request is cached and to release the cache entry nit: this comment ties this to the actual implementation details of the HttpCache::Transaction so it is better if we don't do that. At a high level what we are doing is stating that we are done with the transaction and there won't be any subsequent call to it (as opposed to an abrupt termination caused by an error). https://codereview.chromium.org/23710059/diff/11001/net/url_request/url_reque... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/23710059/diff/11001/net/url_request/url_reque... net/url_request/url_request_unittest.cc:4206: TEST_F(URLRequestTestHTTP, CachedRedirect) { I would have gone for an URLRequestJob unit test... the URLRequest behavior is not changing, and there's no need to start a server
https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_trans... File net/http/http_cache_transaction.cc (left): https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_trans... net/http/http_cache_transaction.cc:453: DCHECK_NE(mode_, UPDATE); On 2013/09/17 19:53:25, rvargas wrote: > Is this line failing? UPDATE means that the caller is attempting manual > revalidation, and the transaction is not even able to read the data, just the > headers. > > Being here for UPDATE means that something is weird and most likely someone is > doing something unexpected. Maybe it triggers because line 1346 is gone... Hrm. Yeah, I'm not sure why I removed it... I think I was confused about the state machine and thought it was actually legitimate state to be in here. Keeping it in there doesn't break net_unittests for me, and makes sense on a second read through the code. I've put it back. https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_trans... net/http/http_cache_transaction.cc:1346: DoneWritingToEntry(true); On 2013/09/17 19:53:25, rvargas wrote: > Even though it is redundant in the sense that the job _should_ call DoneReading, > it is better to keep this logic here so that we don't depend on the job issuing > that call. My reasoning for removing it was that this way we have only one place that declares redirects have no response data. Also redirects are something that happens across transactions rather than within a single one, so moving it out of HttpCache::Transaction seemed cleaner. I don't feel very strongly though. I can put it back if you prefer. https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_unitt... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:5326: TEST(HttpCache, CachedRedirect) { On 2013/09/17 19:53:25, rvargas wrote: > I think is better to write a new test closer to the actual behavior change: now > calling DoneReading() before Read() should release a transaction. Something > close to HttpCache.FilterCompletion but without deleting the first transaction. Added an HttpCache.DiscardedResponse. Did you mean that it should replace URLRequestTestHTTP.DeferredRedirect_ReleaseCacheEntry? (It's still in there for now.) https://codereview.chromium.org/23710059/diff/11001/net/http/http_transaction.h File net/http/http_transaction.h (right): https://codereview.chromium.org/23710059/diff/11001/net/http/http_transaction... net/http/http_transaction.h:112: // This is used to inform the cache that the response was not canceled On 2013/09/17 19:53:25, rvargas wrote: > nit: remove this sentence... or replace "cache" with transaction and "response" > with something else :(. (the response is not canceled, it is the transaction). Done. https://codereview.chromium.org/23710059/diff/11001/net/url_request/url_reque... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/23710059/diff/11001/net/url_request/url_reque... net/url_request/url_request_job.cc:323: // both so the request is cached and to release the cache entry On 2013/09/17 19:53:25, rvargas wrote: > nit: this comment ties this to the actual implementation details of the > HttpCache::Transaction so it is better if we don't do that. At a high level what > we are doing is stating that we are done with the transaction and there won't be > any subsequent call to it (as opposed to an abrupt termination caused by an > error). Done. https://codereview.chromium.org/23710059/diff/11001/net/url_request/url_reque... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/23710059/diff/11001/net/url_request/url_reque... net/url_request/url_request_unittest.cc:4206: TEST_F(URLRequestTestHTTP, CachedRedirect) { On 2013/09/17 19:53:25, rvargas wrote: > I would have gone for an URLRequestJob unit test... the URLRequest behavior is > not changing, and there's no need to start a server Done.
https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_trans... File net/http/http_cache_transaction.cc (left): https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_trans... net/http/http_cache_transaction.cc:1346: DoneWritingToEntry(true); On 2013/09/17 22:16:04, David Benjamin wrote: > On 2013/09/17 19:53:25, rvargas wrote: > > Even though it is redundant in the sense that the job _should_ call > DoneReading, > > it is better to keep this logic here so that we don't depend on the job > issuing > > that call. > > My reasoning for removing it was that this way we have only one place that > declares redirects have no response data. Also redirects are something that > happens across transactions rather than within a single one, so moving it out of > HttpCache::Transaction seemed cleaner. > > I don't feel very strongly though. I can put it back if you prefer. I see your point. On the other hand, having to rely on the caller doing something in order to cache a resource as opposed to making the decision by itself seems weird. It is a trivial check to do, and leaves DoneReading as an optimization as opposed to requirement. I actually prefer leaving this check in. https://codereview.chromium.org/23710059/diff/29001/net/http/http_cache_unitt... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/23710059/diff/29001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:514: const MockTransaction kDiscardedResponse_Transaction = { If we keep this transaction, move it to right before the test (there are no multiple users, and it is simpler to see what the test does) https://codereview.chromium.org/23710059/diff/29001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:526: DiscardedResponseHandler, From the point of view of the test, or the contract change of the transaction, it really doesn't matter if this is a redirect or not, or if the server can return a response body or not. Seems better to just use "" here. https://codereview.chromium.org/23710059/diff/29001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:5386: trans->DoneReading(); This would not be needed. https://codereview.chromium.org/23710059/diff/29001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:5882: TEST(HttpCache, DiscardedResponse) { nit: DiscardedResponse -> DoneReading ? https://codereview.chromium.org/23710059/diff/29001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:5891: MockHttpRequest request(kDiscardedResponse_Transaction); I would prefer using kSimpleGET_Transaction and a ScopedMockTransaction. The point being to avoid distraction from what is relevant for this particular test... it makes it clear that the request doesn't matter (POST, GET, redirect etc). The only thing required for line 5902 to work is an empty response, and doing that for a ScopedMockTransaction is a single line. https://codereview.chromium.org/23710059/diff/29001/net/url_request/url_reque... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/23710059/diff/29001/net/url_request/url_reque... net/url_request/url_request_unittest.cc:4301: TEST_F(URLRequestTestHTTP, DeferredRedirect_ReleaseCacheEntry) { Yes, I meant removing this test.
https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_trans... File net/http/http_cache_transaction.cc (left): https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_trans... net/http/http_cache_transaction.cc:1346: DoneWritingToEntry(true); On 2013/09/18 18:37:10, rvargas wrote: > On 2013/09/17 22:16:04, David Benjamin wrote: > > On 2013/09/17 19:53:25, rvargas wrote: > > > Even though it is redundant in the sense that the job _should_ call > > DoneReading, > > > it is better to keep this logic here so that we don't depend on the job > > issuing > > > that call. > > > > My reasoning for removing it was that this way we have only one place that > > declares redirects have no response data. Also redirects are something that > > happens across transactions rather than within a single one, so moving it out > of > > HttpCache::Transaction seemed cleaner. > > > > I don't feel very strongly though. I can put it back if you prefer. > > I see your point. On the other hand, having to rely on the caller doing > something in order to cache a resource as opposed to making the decision by > itself seems weird. It is a trivial check to do, and leaves DoneReading as an > optimization as opposed to requirement. > > I actually prefer leaving this check in. We don't have to have the caller do anything but read the body to cache the resource. Should the cache know the caller, on redirect, won't bother to read the body? That seems like a decision that should be up to the caller, not the cache.
https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_trans... File net/http/http_cache_transaction.cc (left): https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_trans... net/http/http_cache_transaction.cc:1346: DoneWritingToEntry(true); On 2013/09/18 18:40:39, mmenke wrote: > On 2013/09/18 18:37:10, rvargas wrote: > > On 2013/09/17 22:16:04, David Benjamin wrote: > > > On 2013/09/17 19:53:25, rvargas wrote: > > > > Even though it is redundant in the sense that the job _should_ call > > > DoneReading, > > > > it is better to keep this logic here so that we don't depend on the job > > > issuing > > > > that call. > > > > > > My reasoning for removing it was that this way we have only one place that > > > declares redirects have no response data. Also redirects are something that > > > happens across transactions rather than within a single one, so moving it > out > > of > > > HttpCache::Transaction seemed cleaner. > > > > > > I don't feel very strongly though. I can put it back if you prefer. > > > > I see your point. On the other hand, having to rely on the caller doing > > something in order to cache a resource as opposed to making the decision by > > itself seems weird. It is a trivial check to do, and leaves DoneReading as an > > optimization as opposed to requirement. > > > > I actually prefer leaving this check in. > > We don't have to have the caller do anything but read the body to cache the > resource. Should the cache know the caller, on redirect, won't bother to read > the body? That seems like a decision that should be up to the caller, not the > cache. Here we are doing two things: If we know this is a redirect, we don't bother to store the response body regardless of the caller reading it or not. That's a small optimization that seems completely internal to the cache. Granted, it is small enough to consider removing it. The second thing is to expect a further call from the caller or not, in order to detect errors coming from higher layers. There, the regular contract is that the transaction should be read until we return 0. If that doesn't happen, we assume that there was an error and we should figure out if we want to discard the entry, mark it as truncated, or mark it as good. From this point of view, DoneReading is a signal that tells the cache that the result was OK, but the cache still has heuristics to determine by itself if the resource should be discarded in the absence of that signal. One of those heuristics is having received part of the response body (aka, having something valuable to preserve). This is another part of that heuristic: for a redirect, having part of the body is not significant. We are not removing all the heuristics and replacing them with expecting DoneReading to be called (and we cannot, because we still need a default if DoneReading is not called). Removing this part of the heuristic seems to me like formalizing a contract between the transaction and the job that says that caching of a redirect is completely under control of the job, that not being the case for any other return code.
https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_trans... File net/http/http_cache_transaction.cc (left): https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_trans... net/http/http_cache_transaction.cc:1346: DoneWritingToEntry(true); On 2013/09/18 19:09:12, rvargas wrote: > On 2013/09/18 18:40:39, mmenke wrote: > > On 2013/09/18 18:37:10, rvargas wrote: > > > On 2013/09/17 22:16:04, David Benjamin wrote: > > > > On 2013/09/17 19:53:25, rvargas wrote: > > > > > Even though it is redundant in the sense that the job _should_ call > > > > DoneReading, > > > > > it is better to keep this logic here so that we don't depend on the job > > > > issuing > > > > > that call. > > > > > > > > My reasoning for removing it was that this way we have only one place that > > > > declares redirects have no response data. Also redirects are something > that > > > > happens across transactions rather than within a single one, so moving it > > out > > > of > > > > HttpCache::Transaction seemed cleaner. > > > > > > > > I don't feel very strongly though. I can put it back if you prefer. > > > > > > I see your point. On the other hand, having to rely on the caller doing > > > something in order to cache a resource as opposed to making the decision by > > > itself seems weird. It is a trivial check to do, and leaves DoneReading as > an > > > optimization as opposed to requirement. > > > > > > I actually prefer leaving this check in. Well, it's a requirement to release the cache entry. :-) Is it not also a requirement in the filter case though? Maybe I'm misunderstanding the original purpose of DoneReading. > > We don't have to have the caller do anything but read the body to cache the > > resource. Should the cache know the caller, on redirect, won't bother to read > > the body? That seems like a decision that should be up to the caller, not the > > cache. > > Here we are doing two things: If we know this is a redirect, we don't bother to > store the response body regardless of the caller reading it or not. That's a > small optimization that seems completely internal to the cache. Granted, it is > small enough to consider removing it. But it's not completely internal to the cache, right? Although for this run, whether the body is cached or not does not affect the current caller, it affects the next caller. If the next caller happens to want to read it, they'll get something empty instead of real body. Which is why I thought it would be better if all the logic for whether a response body is read or not was in one place. > The second thing is to expect a further call from the caller or not, in order to > detect errors coming from higher layers. There, the regular contract is that the > transaction should be read until we return 0. If that doesn't happen, we assume > that there was an error and we should figure out if we want to discard the > entry, mark it as truncated, or mark it as good. From this point of view, > DoneReading is a signal that tells the cache that the result was OK, but the > cache still has heuristics to determine by itself if the resource should be > discarded in the absence of that signal. > > One of those heuristics is having received part of the response body (aka, > having something valuable to preserve). This is another part of that heuristic: > for a redirect, having part of the body is not significant. > > We are not removing all the heuristics and replacing them with expecting > DoneReading to be called (and we cannot, because we still need a default if > DoneReading is not called). > > Removing this part of the heuristic seems to me like formalizing a contract > between the transaction and the job that says that caching of a redirect is > completely under control of the job, that not being the case for any other > return code. Isn't it then similarly under control of the job for filters, where DoneReading is also called? I reused DoneReading here just because it seemed like a similar signal; the filter logic knows the response body is useless past a certain point and, likewise, the redirect logic knows the response body is useless past a certain point that happens to be its start. It does seem weird to me that, in both cases, the information about where a request's response body effectively ends is at a higher level than the cache, rather than a lower level, but yeah. https://codereview.chromium.org/23710059/diff/29001/net/http/http_cache_unitt... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/23710059/diff/29001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:514: const MockTransaction kDiscardedResponse_Transaction = { On 2013/09/18 18:37:10, rvargas wrote: > If we keep this transaction, move it to right before the test (there are no > multiple users, and it is simpler to see what the test does) (Removed it) https://codereview.chromium.org/23710059/diff/29001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:526: DiscardedResponseHandler, On 2013/09/18 18:37:10, rvargas wrote: > From the point of view of the test, or the contract change of the transaction, > it really doesn't matter if this is a redirect or not, or if the server can > return a response body or not. Seems better to just use "" here. (Removed it) https://codereview.chromium.org/23710059/diff/29001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:5386: trans->DoneReading(); On 2013/09/18 18:37:10, rvargas wrote: > This would not be needed. (Left this in here for now pending discussion about the redirect check in the cache. I'll get rid of this if we put the check back.) https://codereview.chromium.org/23710059/diff/29001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:5882: TEST(HttpCache, DiscardedResponse) { On 2013/09/18 18:37:10, rvargas wrote: > nit: DiscardedResponse -> DoneReading ? Done. https://codereview.chromium.org/23710059/diff/29001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:5891: MockHttpRequest request(kDiscardedResponse_Transaction); On 2013/09/18 18:37:10, rvargas wrote: > I would prefer using kSimpleGET_Transaction and a ScopedMockTransaction. The > point being to avoid distraction from what is relevant for this particular > test... it makes it clear that the request doesn't matter (POST, GET, redirect > etc). The only thing required for line 5902 to work is an empty response, and > doing that for a ScopedMockTransaction is a single line. Done. https://codereview.chromium.org/23710059/diff/29001/net/url_request/url_reque... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/23710059/diff/29001/net/url_request/url_reque... net/url_request/url_request_unittest.cc:4301: TEST_F(URLRequestTestHTTP, DeferredRedirect_ReleaseCacheEntry) { On 2013/09/18 18:37:10, rvargas wrote: > Yes, I meant removing this test. Done.
https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_trans... File net/http/http_cache_transaction.cc (left): https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_trans... net/http/http_cache_transaction.cc:1346: DoneWritingToEntry(true); On 2013/09/18 19:09:12, rvargas wrote: > On 2013/09/18 18:40:39, mmenke wrote: > > On 2013/09/18 18:37:10, rvargas wrote: > > > On 2013/09/17 22:16:04, David Benjamin wrote: > > > > On 2013/09/17 19:53:25, rvargas wrote: > > > > > Even though it is redundant in the sense that the job _should_ call > > > > DoneReading, > > > > > it is better to keep this logic here so that we don't depend on the job > > > > issuing > > > > > that call. > > > > > > > > My reasoning for removing it was that this way we have only one place that > > > > declares redirects have no response data. Also redirects are something > that > > > > happens across transactions rather than within a single one, so moving it > > out > > > of > > > > HttpCache::Transaction seemed cleaner. > > > > > > > > I don't feel very strongly though. I can put it back if you prefer. > > > > > > I see your point. On the other hand, having to rely on the caller doing > > > something in order to cache a resource as opposed to making the decision by > > > itself seems weird. It is a trivial check to do, and leaves DoneReading as > an > > > optimization as opposed to requirement. > > > > > > I actually prefer leaving this check in. > > > > We don't have to have the caller do anything but read the body to cache the > > resource. Should the cache know the caller, on redirect, won't bother to read > > the body? That seems like a decision that should be up to the caller, not the > > cache. > > Here we are doing two things: If we know this is a redirect, we don't bother to > store the response body regardless of the caller reading it or not. That's a > small optimization that seems completely internal to the cache. Granted, it is > small enough to consider removing it. > > The second thing is to expect a further call from the caller or not, in order to > detect errors coming from higher layers. There, the regular contract is that the > transaction should be read until we return 0. If that doesn't happen, we assume > that there was an error and we should figure out if we want to discard the > entry, mark it as truncated, or mark it as good. From this point of view, > DoneReading is a signal that tells the cache that the result was OK, but the > cache still has heuristics to determine by itself if the resource should be > discarded in the absence of that signal. The old contract is effectively, "After receiving the headers, if it's a redirect, we're done. Otherwise, we read until we get back a value <= 0, or until we abort." Normally if we abort, I'm assuming the cache assumes the transfer was incomplete, so we effectively don't cache the result. This CL moves/copies that "We never care about (or even try to read) the body of a redirect" logic into the UrlRequestJob instead of the HttpCacheTransaction. While I defer to you on this, I'm not really a fan of having that assumption duplicated at two different layers. > One of those heuristics is having received part of the response body (aka, > having something valuable to preserve). This is another part of that heuristic: > for a redirect, having part of the body is not significant. > > We are not removing all the heuristics and replacing them with expecting > DoneReading to be called (and we cannot, because we still need a default if > DoneReading is not called). > > Removing this part of the heuristic seems to me like formalizing a contract > between the transaction and the job that says that caching of a redirect is > completely under control of the job, that not being the case for any other > return code.
LGTM https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_trans... File net/http/http_cache_transaction.cc (left): https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_trans... net/http/http_cache_transaction.cc:1346: DoneWritingToEntry(true); On 2013/09/18 19:38:31, mmenke wrote: > On 2013/09/18 19:09:12, rvargas wrote: > > On 2013/09/18 18:40:39, mmenke wrote: > > > On 2013/09/18 18:37:10, rvargas wrote: > > > > On 2013/09/17 22:16:04, David Benjamin wrote: > > > > > On 2013/09/17 19:53:25, rvargas wrote: > > > > > > Even though it is redundant in the sense that the job _should_ call > > > > > DoneReading, > > > > > > it is better to keep this logic here so that we don't depend on the > job > > > > > issuing > > > > > > that call. > > > > > > > > > > My reasoning for removing it was that this way we have only one place > that > > > > > declares redirects have no response data. Also redirects are something > > that > > > > > happens across transactions rather than within a single one, so moving > it > > > out > > > > of > > > > > HttpCache::Transaction seemed cleaner. > > > > > > > > > > I don't feel very strongly though. I can put it back if you prefer. > > > > > > > > I see your point. On the other hand, having to rely on the caller doing > > > > something in order to cache a resource as opposed to making the decision > by > > > > itself seems weird. It is a trivial check to do, and leaves DoneReading as > > an > > > > optimization as opposed to requirement. > > > > > > > > I actually prefer leaving this check in. > > > > > > We don't have to have the caller do anything but read the body to cache the > > > resource. Should the cache know the caller, on redirect, won't bother to > read > > > the body? That seems like a decision that should be up to the caller, not > the > > > cache. > > > > Here we are doing two things: If we know this is a redirect, we don't bother > to > > store the response body regardless of the caller reading it or not. That's a > > small optimization that seems completely internal to the cache. Granted, it is > > small enough to consider removing it. > > > > The second thing is to expect a further call from the caller or not, in order > to > > detect errors coming from higher layers. There, the regular contract is that > the > > transaction should be read until we return 0. If that doesn't happen, we > assume > > that there was an error and we should figure out if we want to discard the > > entry, mark it as truncated, or mark it as good. From this point of view, > > DoneReading is a signal that tells the cache that the result was OK, but the > > cache still has heuristics to determine by itself if the resource should be > > discarded in the absence of that signal. > > The old contract is effectively, "After receiving the headers, if it's a > redirect, we're done. Otherwise, we read until we get back a value <= 0, or > until we abort." sure, but what the cache keeps or drops is not part of the contract so it is free to inspect whatever it wants to decide what to do. > Normally if we abort, I'm assuming the cache assumes the > transfer was incomplete, so we effectively don't cache the result. > > This CL moves/copies that "We never care about (or even try to read) the body of > a redirect" logic into the UrlRequestJob instead of the HttpCacheTransaction. > While I defer to you on this, I'm not really a fan of having that assumption > duplicated at two different layers. > > > One of those heuristics is having received part of the response body (aka, > > having something valuable to preserve). This is another part of that > heuristic: > > for a redirect, having part of the body is not significant. > > > > We are not removing all the heuristics and replacing them with expecting > > DoneReading to be called (and we cannot, because we still need a default if > > DoneReading is not called). > > > > Removing this part of the heuristic seems to me like formalizing a contract > > between the transaction and the job that says that caching of a redirect is > > completely under control of the job, that not being the case for any other > > return code. > https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_trans... net/http/http_cache_transaction.cc:1346: DoneWritingToEntry(true); On 2013/09/18 19:35:43, David Benjamin wrote: > On 2013/09/18 19:09:12, rvargas wrote: > > On 2013/09/18 18:40:39, mmenke wrote: > > > On 2013/09/18 18:37:10, rvargas wrote: > > > > On 2013/09/17 22:16:04, David Benjamin wrote: > > > > > On 2013/09/17 19:53:25, rvargas wrote: > > > > > > Even though it is redundant in the sense that the job _should_ call > > > > > DoneReading, > > > > > > it is better to keep this logic here so that we don't depend on the > job > > > > > issuing > > > > > > that call. > > > > > > > > > > My reasoning for removing it was that this way we have only one place > that > > > > > declares redirects have no response data. Also redirects are something > > that > > > > > happens across transactions rather than within a single one, so moving > it > > > out > > > > of > > > > > HttpCache::Transaction seemed cleaner. > > > > > > > > > > I don't feel very strongly though. I can put it back if you prefer. > > > > > > > > I see your point. On the other hand, having to rely on the caller doing > > > > something in order to cache a resource as opposed to making the decision > by > > > > itself seems weird. It is a trivial check to do, and leaves DoneReading as > > an > > > > optimization as opposed to requirement. > > > > > > > > I actually prefer leaving this check in. > > Well, it's a requirement to release the cache entry. :-) > > Is it not also a requirement in the filter case though? Maybe I'm > misunderstanding the original purpose of DoneReading. Sort of. The contract for releasing the entry is that the transaction writing to it should be cancelled. I wouldn't really want to change the meaning of "I'm not going to read anymore" to be "release the entry for the next reader". In the case of filters, the result of not calling DoneReading() is that the resource will be marked as truncated (assuming there is something on the body), and the next user will have to revalidate and attempt to complete the download. That may or may not result in an unusable cached resource (as in it may not be possible to do a revalidation), so it _may_ degenerate into the same behavior for redirects (where we would drop the cached response). > > > > > We don't have to have the caller do anything but read the body to cache the > > > resource. Should the cache know the caller, on redirect, won't bother to > read > > > the body? That seems like a decision that should be up to the caller, not > the > > > cache. > > > > Here we are doing two things: If we know this is a redirect, we don't bother > to > > store the response body regardless of the caller reading it or not. That's a > > small optimization that seems completely internal to the cache. Granted, it is > > small enough to consider removing it. > > But it's not completely internal to the cache, right? Although for this run, > whether the body is cached or not does not affect the current caller, it affects > the next caller. If the next caller happens to want to read it, they'll get > something empty instead of real body. Which is why I thought it would be better > if all the logic for whether a response body is read or not was in one place. That's a very good point. It would be bad to skip the body if the caller wants it for some reason. off with the check! > > > > The second thing is to expect a further call from the caller or not, in order > to > > detect errors coming from higher layers. There, the regular contract is that > the > > transaction should be read until we return 0. If that doesn't happen, we > assume > > that there was an error and we should figure out if we want to discard the > > entry, mark it as truncated, or mark it as good. From this point of view, > > DoneReading is a signal that tells the cache that the result was OK, but the > > cache still has heuristics to determine by itself if the resource should be > > discarded in the absence of that signal. > > > > One of those heuristics is having received part of the response body (aka, > > having something valuable to preserve). This is another part of that > heuristic: > > for a redirect, having part of the body is not significant. > > > > We are not removing all the heuristics and replacing them with expecting > > DoneReading to be called (and we cannot, because we still need a default if > > DoneReading is not called). > > > > Removing this part of the heuristic seems to me like formalizing a contract > > between the transaction and the job that says that caching of a redirect is > > completely under control of the job, that not being the case for any other > > return code. > > Isn't it then similarly under control of the job for filters, where DoneReading > is also called? I reused DoneReading here just because it seemed like a similar > signal; the filter logic knows the response body is useless past a certain point > and, likewise, the redirect logic knows the response body is useless past a > certain point that happens to be its start. It does seem weird to me that, in > both cases, the information about where a request's response body effectively > ends is at a higher level than the cache, rather than a lower level, but yeah.
LGTM, either way. https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_trans... File net/http/http_cache_transaction.cc (left): https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_trans... net/http/http_cache_transaction.cc:1346: DoneWritingToEntry(true); On 2013/09/18 21:41:32, rvargas wrote: > On 2013/09/18 19:38:31, mmenke wrote: > > On 2013/09/18 19:09:12, rvargas wrote: > > > On 2013/09/18 18:40:39, mmenke wrote: > > > > On 2013/09/18 18:37:10, rvargas wrote: > > > > > On 2013/09/17 22:16:04, David Benjamin wrote: > > > > > > On 2013/09/17 19:53:25, rvargas wrote: > > > > > > > Even though it is redundant in the sense that the job _should_ call > > > > > > DoneReading, > > > > > > > it is better to keep this logic here so that we don't depend on the > > job > > > > > > issuing > > > > > > > that call. > > > > > > > > > > > > My reasoning for removing it was that this way we have only one place > > that > > > > > > declares redirects have no response data. Also redirects are something > > > that > > > > > > happens across transactions rather than within a single one, so moving > > it > > > > out > > > > > of > > > > > > HttpCache::Transaction seemed cleaner. > > > > > > > > > > > > I don't feel very strongly though. I can put it back if you prefer. > > > > > > > > > > I see your point. On the other hand, having to rely on the caller doing > > > > > something in order to cache a resource as opposed to making the decision > > by > > > > > itself seems weird. It is a trivial check to do, and leaves DoneReading > as > > > an > > > > > optimization as opposed to requirement. > > > > > > > > > > I actually prefer leaving this check in. > > > > > > > > We don't have to have the caller do anything but read the body to cache > the > > > > resource. Should the cache know the caller, on redirect, won't bother to > > read > > > > the body? That seems like a decision that should be up to the caller, not > > the > > > > cache. > > > > > > Here we are doing two things: If we know this is a redirect, we don't bother > > to > > > store the response body regardless of the caller reading it or not. That's a > > > small optimization that seems completely internal to the cache. Granted, it > is > > > small enough to consider removing it. > > > > > > The second thing is to expect a further call from the caller or not, in > order > > to > > > detect errors coming from higher layers. There, the regular contract is that > > the > > > transaction should be read until we return 0. If that doesn't happen, we > > assume > > > that there was an error and we should figure out if we want to discard the > > > entry, mark it as truncated, or mark it as good. From this point of view, > > > DoneReading is a signal that tells the cache that the result was OK, but the > > > cache still has heuristics to determine by itself if the resource should be > > > discarded in the absence of that signal. > > > > The old contract is effectively, "After receiving the headers, if it's a > > redirect, we're done. Otherwise, we read until we get back a value <= 0, or > > until we abort." > > sure, but what the cache keeps or drops is not part of the contract so it is > free to inspect whatever it wants to decide what to do. I think falsely claiming an HTTP redirect response has a 0-length body does call the contract into question a bit.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidben@chromium.org/23710059/45001
Message was sent while issue was closed.
Change committed as 224161 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
