Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
403393003:
HTTP retry support.
DescriptionHTTP retry support.
If you are connected to the Internet through a connection
with a high error rate you can often experience TCP
connections being reset. At times completing a page load
can be a challenge and you have to reload a page multiple
times to get the page loaded completely.
This CL will attempt to retry GET requests up to five
times. Content is hashed and hash verified on retry. Byte
range requests are supported, but disabled for now.
BUG=397474
Patch Set 1 #
Total comments: 27
Patch Set 2 : Fixed according to comments. #
Total comments: 45
Patch Set 3 : Fixes according to review. #
Total comments: 2
Messages
Total messages: 13 (1 generated)
@reviewer: does this patch look OK to you?
All of the header logic makes me very nervous. https://codereview.chromium.org/403393003/diff/1/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/403393003/diff/1/net/base/net_error_list.h#ne... net/base/net_error_list.h:621: NET_ERROR(RETRY_CONTENT_UPDATED, -366) Should add a comment that these are just temporary error codes (If we ended up keeping them, would need error pages for them). https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... net/http/http_network_transaction.cc:78: // Array of all network error codes. Needed for histograms. Not needed. See GetAllErrorCodesForUma (declared in net_errors.h) https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... net/http/http_network_transaction.cc:1263: if (request_->load_flags & LOAD_MAIN_FRAME) Use braces when the body of an if takes more than one line. https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... net/http/http_network_transaction.cc:1268: kAllNetErrorCodes, arraysize(kAllNetErrorCodes))); Should indent lines that continue statements by 4 lines. SHould be something like: UMA_HISTOGRAM_CUSTOM_ENUMERATION( "Net.HttpRetry.Cause.MainFrame", -error, base::CustomHistogram::ArrayToCustomRanges( kAllNetErrorCodes, arraysize(kAllNetErrorCodes))); https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... net/http/http_network_transaction.cc:1514: if (request_->method != "GET" || retry_attempt_ > 3) Not retrying on POSTs when we received a connection error on a reused socket is a bad idea. Servers may timeout idle sockets at any time. So if we ever want to be able to reuse sockets for POSTs at all, we have to retry POSTs. https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... net/http/http_network_transaction.cc:1514: if (request_->method != "GET" || retry_attempt_ > 3) The 3 should be a constant at the top of this file. https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... net/http/http_network_transaction.cc:1517: if (headers_valid_) { Your should have some sort of time check here. If we get a timeout after waiting for 30 seconds without receiving data, we probably don't want to retry, unless perhaps it was a stale socket. https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... net/http/http_network_transaction.cc:1523: // If etag or last-modified is set we can always retry since the responce response https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... net/http/http_network_transaction.cc:1527: return true; Use braces when the condition of an if statement takes multiple times. https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... net/http/http_network_transaction.cc:1531: return false; Not a big fan of having all this cache logic duplicated here and in the caching code. Seems ugly and regression prone. You're missing "no-store", for instance. https://codereview.chromium.org/403393003/diff/1/net/http/http_stream_base.h File net/http/http_stream_base.h (right): https://codereview.chromium.org/403393003/diff/1/net/http/http_stream_base.h#... net/http/http_stream_base.h:126: virtual int64 GetReceivedBodyLength() const = 0; Why are these in the HttpStreamBase class, and not in HttpNetworkTransaction? Then you get QUIC and SPDY support for free, if we want them. https://codereview.chromium.org/403393003/diff/1/net/spdy/spdy_http_stream.cc File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/403393003/diff/1/net/spdy/spdy_http_stream.cc... net/spdy/spdy_http_stream.cc:187: int64 SpdyHttpStream::GetReceivedBodyLength() const { This seems to violate the API.
https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... net/http/http_network_transaction.cc:1467: case ERR_EMPTY_RESPONSE: Note that this is a conservative list of all "expected" errors - errors that we could legitimately get when trying to reuse a socket, even if everything's just fine and dandy. Depending on the expected use case of retrying more aggressively, may well want to expand the list.
Thanks Matt, now improved according to your review. https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... net/http/http_network_transaction.cc:78: // Array of all network error codes. Needed for histograms. On 2014/07/22 14:57:41, mmenke wrote: > Not needed. See GetAllErrorCodesForUma (declared in net_errors.h) Done. https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... net/http/http_network_transaction.cc:1263: if (request_->load_flags & LOAD_MAIN_FRAME) On 2014/07/22 14:57:41, mmenke wrote: > Use braces when the body of an if takes more than one line. Done. https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... net/http/http_network_transaction.cc:1268: kAllNetErrorCodes, arraysize(kAllNetErrorCodes))); On 2014/07/22 14:57:41, mmenke wrote: > Should indent lines that continue statements by 4 lines. SHould be something > like: > > UMA_HISTOGRAM_CUSTOM_ENUMERATION( > "Net.HttpRetry.Cause.MainFrame", > -error, > base::CustomHistogram::ArrayToCustomRanges( > kAllNetErrorCodes, arraysize(kAllNetErrorCodes))); Done. https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... net/http/http_network_transaction.cc:1467: case ERR_EMPTY_RESPONSE: On 2014/07/22 15:01:41, mmenke wrote: > Note that this is a conservative list of all "expected" errors - errors that we > could legitimately get when trying to reuse a socket, even if everything's just > fine and dandy. > > Depending on the expected use case of retrying more aggressively, may well want > to expand the list. Agreed, ERR_CONTENT_LENGTH_MISMATCH should probably be added to the list, at least if we have received less than expected data. https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... net/http/http_network_transaction.cc:1514: if (request_->method != "GET" || retry_attempt_ > 3) On 2014/07/22 14:57:41, mmenke wrote: > Not retrying on POSTs when we received a connection error on a reused socket is > a bad idea. > > Servers may timeout idle sockets at any time. So if we ever want to be able to > reuse sockets for POSTs at all, we have to retry POSTs. Done. https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... net/http/http_network_transaction.cc:1514: if (request_->method != "GET" || retry_attempt_ > 3) On 2014/07/22 14:57:41, mmenke wrote: > The 3 should be a constant at the top of this file. Done. https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... net/http/http_network_transaction.cc:1514: if (request_->method != "GET" || retry_attempt_ > 3) On 2014/07/22 14:57:41, mmenke wrote: > The 3 should be a constant at the top of this file. Done. https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... net/http/http_network_transaction.cc:1514: if (request_->method != "GET" || retry_attempt_ > 3) On 2014/07/22 14:57:41, mmenke wrote: > Not retrying on POSTs when we received a connection error on a reused socket is > a bad idea. > > Servers may timeout idle sockets at any time. So if we ever want to be able to > reuse sockets for POSTs at all, we have to retry POSTs. Done. https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... net/http/http_network_transaction.cc:1523: // If etag or last-modified is set we can always retry since the responce On 2014/07/22 14:57:41, mmenke wrote: > response Done. https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... net/http/http_network_transaction.cc:1527: return true; On 2014/07/22 14:57:41, mmenke wrote: > Use braces when the condition of an if statement takes multiple times. Done. https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... net/http/http_network_transaction.cc:1531: return false; On 2014/07/22 14:57:41, mmenke wrote: > Not a big fan of having all this cache logic duplicated here and in the caching > code. Seems ugly and regression prone. You're missing "no-store", for > instance. I did not like it either. I also missed must-revalidate I guess. But I can not reuse the cache logic directly, since the behavior here is slightly different. Suggestions for how to handle it better would be appreciated. https://codereview.chromium.org/403393003/diff/1/net/http/http_stream_base.h File net/http/http_stream_base.h (right): https://codereview.chromium.org/403393003/diff/1/net/http/http_stream_base.h#... net/http/http_stream_base.h:126: virtual int64 GetReceivedBodyLength() const = 0; On 2014/07/22 14:57:41, mmenke wrote: > Why are these in the HttpStreamBase class, and not in HttpNetworkTransaction? > Then you get QUIC and SPDY support for free, if we want them. I could not do this in HttpNetworkTransaction since io_callback_ is used for returning data. No actual data passes through HttpNetworkTransaction as far as I can see.
[+rvargas] for thoughts on the cache/header logic in HttpNetworkTransaction::ShouldResendRequest, which I'd really prefer not to see duplicated. jonnyr: I'll do another pass later today. https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... net/http/http_network_transaction.cc:1531: return false; On 2014/07/23 08:43:04, jonnyr wrote: > On 2014/07/22 14:57:41, mmenke wrote: > > Not a big fan of having all this cache logic duplicated here and in the > caching > > code. Seems ugly and regression prone. You're missing "no-store", for > > instance. > > I did not like it either. I also missed must-revalidate I guess. > > But I can not reuse the cache logic directly, since the behavior here is > slightly different. Suggestions for how to handle it better would be > appreciated. I'm not at all familiar with caching semantics or the cache code. Maybe rope in rvargas? https://codereview.chromium.org/403393003/diff/1/net/http/http_stream_base.h File net/http/http_stream_base.h (right): https://codereview.chromium.org/403393003/diff/1/net/http/http_stream_base.h#... net/http/http_stream_base.h:126: virtual int64 GetReceivedBodyLength() const = 0; On 2014/07/23 08:43:05, jonnyr wrote: > On 2014/07/22 14:57:41, mmenke wrote: > > Why are these in the HttpStreamBase class, and not in HttpNetworkTransaction? > > Then you get QUIC and SPDY support for free, if we want them. > > I could not do this in HttpNetworkTransaction since io_callback_ is used for > returning data. No actual data passes through HttpNetworkTransaction as far as I > can see. I'm not following why this doesn't work. In HttpNetworkTransaction::DoReadBodyComplete, we have result, which is the number of body bytes read (or an error), and we have |read_buf_|. Am I missing something?
On 2014/07/23 15:08:41, mmenke wrote: > [+rvargas] for thoughts on the cache/header logic in > HttpNetworkTransaction::ShouldResendRequest, which I'd really prefer not to see > duplicated. > > jonnyr: I'll do another pass later today. > > https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... > File net/http/http_network_transaction.cc (right): > > https://codereview.chromium.org/403393003/diff/1/net/http/http_network_transa... > net/http/http_network_transaction.cc:1531: return false; > On 2014/07/23 08:43:04, jonnyr wrote: > > On 2014/07/22 14:57:41, mmenke wrote: > > > Not a big fan of having all this cache logic duplicated here and in the > > caching > > > code. Seems ugly and regression prone. You're missing "no-store", for > > > instance. > > > > I did not like it either. I also missed must-revalidate I guess. > > > > But I can not reuse the cache logic directly, since the behavior here is > > slightly different. Suggestions for how to handle it better would be > > appreciated. > > I'm not at all familiar with caching semantics or the cache code. Maybe rope in > rvargas? > > https://codereview.chromium.org/403393003/diff/1/net/http/http_stream_base.h > File net/http/http_stream_base.h (right): > > https://codereview.chromium.org/403393003/diff/1/net/http/http_stream_base.h#... > net/http/http_stream_base.h:126: virtual int64 GetReceivedBodyLength() const = > 0; > On 2014/07/23 08:43:05, jonnyr wrote: > > On 2014/07/22 14:57:41, mmenke wrote: > > > Why are these in the HttpStreamBase class, and not in > HttpNetworkTransaction? > > > Then you get QUIC and SPDY support for free, if we want them. > > > > I could not do this in HttpNetworkTransaction since io_callback_ is used for > > returning data. No actual data passes through HttpNetworkTransaction as far as > I > > can see. > > I'm not following why this doesn't work. In > HttpNetworkTransaction::DoReadBodyComplete, we have result, which is the number > of body bytes read (or an error), and we have |read_buf_|. Am I missing > something? I think I did. Let me see what I can do about that.
A couple minor comments. Good job on the tests, look pretty thorough. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction.cc:1512: if (headers_valid_) { If there a reason the old GetResponseHeaders() != NULL check doesn't work? https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction.cc:1557: return true; Out of paranoia, think we should keep the only retry non-GETs (And maybe non-HEADs) if the connection was also reused. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:297: size_t read5_count = 0); While the Google style guide does technically allow default arguments in limited situations, including this one, I believe, we generally avoid their use, and use vectors instead. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1373: session_deps_.net_log = &net_log; nit: Don't need these two lines (A lot of tests set up a NetLog unnecessarily). https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1387: StaticSocketDataProvider data2(read2, read2_count, NULL, 0); Hrm...Only checking the write of the first request? https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1404: for (int i = 0; i < 1; ++i) { This loop isn't needed. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1668: MockRead("Cache-Control: max-age=100\r\n"), I don't believe this test should have the max age header - that's checked in the next one. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1699: "Pragma: no-cache\r\n"\ Backslashes aren't needed at the end of these lines. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1706: MockRead("HTTP/1.1 200 OK\r\nPragma: no-cache\r\n"), Pragma: no-cache should be on its own line (Or did you intend to remove it? Think that might be for the best, since it's the first request's header that matters) Actually...we shouldn't even read from this socket again, so there probably shouldn't be a data_reads2. That ensures that we hard fail if we try to create another socket, I believe. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1716: TEST_P(HttpNetworkTransactionTest, PartialDataRetryCacheControlNoCache) { How's this different from the previous test? https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1743: MockRead data_reads2[] = { Again, shouldn't be a second read when we don't use it (Not going to mention this for the other tests it applies to). https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1803: "012345678901234567890123456789012345678901234567890123456789"\ All these backslashes aren't needed. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1804: "0123456789012345678901234567890123456789012345678901234567890"), Suggest splitting this up into multiple reads, in both cases (And maybe divide up the body reads a bit differently in the two sets of reads). https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1828: "01234567890123456789012345678901234567890123456789", For sanity, should use the same number of characters per line here as in the above body strings. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1852: "Content-Range: bytes 1-2/3\r\n\r\n"), Should definitely check the second set of request headers in this test.
Please create a bug for tracking this and format the CL description to 80 cols or so. I just looked at (parts of) one file. I don't see an easy way to share code with the cache given that the logic is not really the same. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction.cc:1513: // Do not attempt a retry on anything but GET request if headers are valid. How do we know that nothing actually reached the server when headers_valid_ is false? https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction.cc:1523: if (response_.headers->GetContentLength() > 1000000) Seems like this should go before the 30 seconds rule. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction.cc:1528: if (response_.headers->HasHeader("etag") || use HasStrongValidators() https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction.cc:1533: if (response_.headers->HasHeaderValue("cache-control", "no-cache") || why fail if no-cache or must-revalidate? https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction.cc:1544: // generated at the time when we received the response, hence do not retry. why not? If it actually differs, we'll catch that by the hash. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction.cc:1554: } Most of this logic should probably be moved to a method of HttpResponseHeaders. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction.cc:1593: if (!previous_etag_.empty()) I suggest checking this one before last-modified and not adding both headers if this one is present.
I created http://code.google.com/p/chromium/issues/detail?id=397474 to track this issue, I hope that is done correctly. There are two things in the current reviews I have not done yet, one is using vectors in the unit test, the other is checking that the requests are correct. This is especially important once range requests are supported. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction.cc:1512: if (headers_valid_) { On 2014/07/23 20:17:42, mmenke wrote: > If there a reason the old GetResponseHeaders() != NULL check doesn't work? Done. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction.cc:1513: // Do not attempt a retry on anything but GET request if headers are valid. On 2014/07/24 19:09:47, rvargas wrote: > How do we know that nothing actually reached the server when headers_valid_ is > false? Right, this will have to be removed. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction.cc:1523: if (response_.headers->GetContentLength() > 1000000) On 2014/07/24 19:09:47, rvargas wrote: > Seems like this should go before the 30 seconds rule. Done. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction.cc:1528: if (response_.headers->HasHeader("etag") || On 2014/07/24 19:09:47, rvargas wrote: > use HasStrongValidators() Done. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction.cc:1533: if (response_.headers->HasHeaderValue("cache-control", "no-cache") || On 2014/07/24 19:09:47, rvargas wrote: > why fail if no-cache or must-revalidate? What I am trying to avoid is that we glue together two responses that have been modified between retry attempts. I am using the cache headers as an indication that the resource could be modified in between two requests in an attempt to be as careful as possible. no-cache is used to force intermediate caches to obtain a new copy from the origin server, and must-revalidate to force the client to revalidate with server if stale. I am thinking that developers use this to get the client to check with original content servers often since content is changed all the time. An alternative to being this strict is to cross-check content, and save and match x number of bytes in the response before glueing together the result. Currently everything is hashed, but the intention is to remove the hashing once we see that we do not get too many ERR_RETRY_HASH_MISMATCH errors. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction.cc:1544: // generated at the time when we received the response, hence do not retry. On 2014/07/24 19:09:48, rvargas wrote: > why not? If it actually differs, we'll catch that by the hash. The intention is to remove the hash. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction.cc:1554: } On 2014/07/24 19:09:47, rvargas wrote: > Most of this logic should probably be moved to a method of HttpResponseHeaders. That I can do. Do you think the overall strategy as mentioned above makes sense? https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction.cc:1557: return true; On 2014/07/23 20:17:42, mmenke wrote: > Out of paranoia, think we should keep the only retry non-GETs (And maybe > non-HEADs) if the connection was also reused. Agreed. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction.cc:1593: if (!previous_etag_.empty()) On 2014/07/24 19:09:47, rvargas wrote: > I suggest checking this one before last-modified and not adding both headers if > this one is present. Done. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1373: session_deps_.net_log = &net_log; On 2014/07/23 20:17:42, mmenke wrote: > nit: Don't need these two lines (A lot of tests set up a NetLog unnecessarily). Done, but meant that I also had to remove TestLoadTimingNotReused. Should not be important for this test though. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1387: StaticSocketDataProvider data2(read2, read2_count, NULL, 0); On 2014/07/23 20:17:43, mmenke wrote: > Hrm...Only checking the write of the first request? Yes, I need to see if I can verify that the retry request is correct somehow. That is currently not done. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1404: for (int i = 0; i < 1; ++i) { On 2014/07/23 20:17:43, mmenke wrote: > This loop isn't needed. Done. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1668: MockRead("Cache-Control: max-age=100\r\n"), On 2014/07/23 20:17:43, mmenke wrote: > I don't believe this test should have the max age header - that's checked in the > next one. This test checks full header without any body data for border cases. Name of test now changed to indicate that. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1699: "Pragma: no-cache\r\n"\ On 2014/07/23 20:17:43, mmenke wrote: > Backslashes aren't needed at the end of these lines. Done. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1706: MockRead("HTTP/1.1 200 OK\r\nPragma: no-cache\r\n"), On 2014/07/23 20:17:43, mmenke wrote: > Pragma: no-cache should be on its own line (Or did you intend to remove it? > Think that might be for the best, since it's the first request's header that > matters) > > Actually...we shouldn't even read from this socket again, so there probably > shouldn't be a data_reads2. That ensures that we hard fail if we try to create > another socket, I believe. Done. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1716: TEST_P(HttpNetworkTransactionTest, PartialDataRetryCacheControlNoCache) { On 2014/07/23 20:17:43, mmenke wrote: > How's this different from the previous test? It is not different anymore. Earlier it went through two different codepaths. Removed now. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1743: MockRead data_reads2[] = { On 2014/07/23 20:17:43, mmenke wrote: > Again, shouldn't be a second read when we don't use it (Not going to mention > this for the other tests it applies to). Done. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1803: "012345678901234567890123456789012345678901234567890123456789"\ On 2014/07/23 20:17:43, mmenke wrote: > All these backslashes aren't needed. Done. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1804: "0123456789012345678901234567890123456789012345678901234567890"), On 2014/07/23 20:17:43, mmenke wrote: > Suggest splitting this up into multiple reads, in both cases (And maybe divide > up the body reads a bit differently in the two sets of reads). Done. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1828: "01234567890123456789012345678901234567890123456789", On 2014/07/23 20:17:42, mmenke wrote: > For sanity, should use the same number of characters per line here as in the > above body strings. Done. https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:1852: "Content-Range: bytes 1-2/3\r\n\r\n"), On 2014/07/23 20:17:43, mmenke wrote: > Should definitely check the second set of request headers in this test. Agreed.
https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction.cc:1533: if (response_.headers->HasHeaderValue("cache-control", "no-cache") || On 2014/07/25 13:41:59, jonnyr wrote: > On 2014/07/24 19:09:47, rvargas wrote: > > why fail if no-cache or must-revalidate? > > What I am trying to avoid is that we glue together two responses > that have been modified between retry attempts. I am using the > cache headers as an indication that the resource could be modified > in between two requests in an attempt to be as careful as possible. > > no-cache is used to force intermediate caches to obtain a new copy > from the origin server, and must-revalidate to force the client to > revalidate with server if stale. I am thinking that developers use > this to get the client to check with original content servers often > since content is changed all the time. > > An alternative to being this strict is to cross-check content, and > save and match x number of bytes in the response before glueing > together the result. Currently everything is hashed, but the > intention is to remove the hashing once we see that we do not get > too many ERR_RETRY_HASH_MISMATCH errors. hmm... I don't think it is safe to remove the hash unless we have zero ERR_RETRY_HASH_MISMATCH, at which point something is probably broken an not reporting that error :) Even if statistically speaking the error rate is insignificant, the fact that we would be mixing responses seems unacceptable to me. And if there is a hash in place, the only reason I could think of for being extra conservative would be to try to avoid extra latency before actually failing a request, but that should probably come out of an experiment (I mean, we are assuming that the user is better served by waiting more and actually seeing the page than a fast failure) I think there's a lot of no-cache content that doesn't really change that often but people still feel the need to force revalidations to avoid showing stale content occasionally. (That is to say, choosing an expiration date is not an easy thing). https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... net/http/http_network_transaction.cc:1554: } On 2014/07/25 13:41:59, jonnyr wrote: > On 2014/07/24 19:09:47, rvargas wrote: > > Most of this logic should probably be moved to a method of > HttpResponseHeaders. > > That I can do. Do you think the overall strategy as mentioned above makes sense? Yeah, I think that avoid merging by using a hash makes sense, and what I've seen seems appropriate. https://codereview.chromium.org/403393003/diff/60001/net/http/http_network_tr... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/403393003/diff/60001/net/http/http_network_tr... net/http/http_network_transaction.cc:1601: !(request_->method == "GET" || request_->method == "HEAD")) nit: requires {} https://codereview.chromium.org/403393003/diff/60001/net/http/http_network_tr... net/http/http_network_transaction.cc:1617: if (response_.headers->HasStrongValidators()) { nit: no {}
On 2014/07/26 00:30:33, rvargas wrote: > https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... > File net/http/http_network_transaction.cc (right): > > https://codereview.chromium.org/403393003/diff/20001/net/http/http_network_tr... > net/http/http_network_transaction.cc:1533: if > (response_.headers->HasHeaderValue("cache-control", "no-cache") || > On 2014/07/25 13:41:59, jonnyr wrote: > > On 2014/07/24 19:09:47, rvargas wrote: > > > why fail if no-cache or must-revalidate? > > > > What I am trying to avoid is that we glue together two responses > > that have been modified between retry attempts. I am using the > > cache headers as an indication that the resource could be modified > > in between two requests in an attempt to be as careful as possible. > > > > no-cache is used to force intermediate caches to obtain a new copy > > from the origin server, and must-revalidate to force the client to > > revalidate with server if stale. I am thinking that developers use > > this to get the client to check with original content servers often > > since content is changed all the time. > > > > An alternative to being this strict is to cross-check content, and > > save and match x number of bytes in the response before glueing > > together the result. Currently everything is hashed, but the > > intention is to remove the hashing once we see that we do not get > > too many ERR_RETRY_HASH_MISMATCH errors. > > hmm... I don't think it is safe to remove the hash unless we have zero > ERR_RETRY_HASH_MISMATCH, at which point something is probably broken an not > reporting that error :) > > Even if statistically speaking the error rate is insignificant, the fact that we > would be mixing responses seems unacceptable to me. > > And if there is a hash in place, the only reason I could think of for being > extra conservative would be to try to avoid extra latency before actually > failing a request, but that should probably come out of an experiment (I mean, > we are assuming that the user is better served by waiting more and actually > seeing the page than a fast failure) Yes, agreed. The only issue I have with keeping the hash support is the actual cost of hashing. I used sha2 for now since that had Pickle support. If we want to optimize further we can avoid hashing resources that have etag/last-modified set. Also it would be possible to switch to ghash since that is much less costly, but then I would need to add Pickle support to that. I guess performance of the current solution would be tested automatically, no need for me to worry about that prematurely though?
mmenke@chromium.org changed reviewers: - mmenke@chromium.org
Removing myself as reviewer. Too many stale CLs on my list. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
