Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22926031:
Sparse IO: Allow failover to network in debug builds (Closed)
|
Created:
7 years, 4 months ago by pasko Modified:
7 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSparse IO: Allow failover to network in debug builds
In case a backend does not support Sparse IO, a failover to network should
happen, it does so via unsuccessful validation of headers, but debug builds have
a DCHECK on this seemingly working path. Remove it.
BUG=263642
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220564
Patch Set 1 #
Total comments: 2
Patch Set 2 : . #
Total comments: 4
Patch Set 3 : . #Patch Set 4 : . #Patch Set 5 : . #Patch Set 6 : . #
Total comments: 11
Patch Set 7 : avoid uninitialized in ::MockDiskEntry #Patch Set 8 : . #Patch Set 9 : two tests #Patch Set 10 : trying another upload, hopefully it will override the previous "old chunk mismatch" #
Messages
Total messages: 28 (0 generated)
Gavin, Ricardo, please review. This is a temporary measure to fix media tests on Android, we can revive the DCHECK once the Simple Backend supports sparse IO.
https://codereview.chromium.org/22926031/diff/1/net/http/http_cache_transacti... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/22926031/diff/1/net/http/http_cache_transacti... net/http/http_cache_transaction.cc:1534: // header validation will fail and transition to STATE_SEND_REQUEST. I would prefer if you return a specific error code from the backend and do an explicit transition to whatever you need here.
PTAL https://codereview.chromium.org/22926031/diff/1/net/http/http_cache_transacti... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/22926031/diff/1/net/http/http_cache_transacti... net/http/http_cache_transaction.cc:1534: // header validation will fail and transition to STATE_SEND_REQUEST. On 2013/08/23 17:54:13, rvargas wrote: > I would prefer if you return a specific error code from the backend and do an > explicit transition to whatever you need here. Done.
LGTM, Ricardo's suggestion really cleaned this up. I'd be happier with a unit test if that can be easily written (I haven't looked), that way we'll remember to remove this when the simple cache backend supports ranges as the unit test hopefully stops passing. But that unit test would be mighty situationally specific, so maybe not. https://codereview.chromium.org/22926031/diff/7001/net/http/http_cache_transa... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/22926031/diff/7001/net/http/http_cache_transa... net/http/http_cache_transaction.cc:1534: // Send the network request if the backend does not support caching ranges. Should we add a TODO here for after the simple cache supports ranges?
Wait: not lgtm. The showstopper here is that we need to #ifdef this code to android specifically, so it can be merged in to m30.
On 2013/08/26 20:24:02, gavinp wrote: > Wait: not lgtm. > > The showstopper here is that we need to #ifdef this code to android > specifically, so it can be merged in to m30. (and I'll repeat my ell gee tee em when I look over that upload, sorry to be supremely paranoid)
https://codereview.chromium.org/22926031/diff/7001/net/http/http_cache_transa... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/22926031/diff/7001/net/http/http_cache_transa... net/http/http_cache_transaction.cc:1535: next_state_ = STATE_SEND_REQUEST; This is most likely not enough. It basically means that the transaction is moving forward to issue the request (as it stands right now) and when the response is received we'll assume that the backend has sparse data or can deal with it. I suspect that either we have to go through DoRestartPartialRequest() (and transition to mode_ = WRITE), or close the entry and transition to pass through (mode_ = NONE) in case we want to spare the entry.
PTAL https://codereview.chromium.org/22926031/diff/7001/net/http/http_cache_transa... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/22926031/diff/7001/net/http/http_cache_transa... net/http/http_cache_transaction.cc:1534: // Send the network request if the backend does not support caching ranges. On 2013/08/26 15:46:37, gavinp wrote: > Should we add a TODO here for after the simple cache supports ranges? Done. https://codereview.chromium.org/22926031/diff/7001/net/http/http_cache_transa... net/http/http_cache_transaction.cc:1535: next_state_ = STATE_SEND_REQUEST; On 2013/08/26 21:24:52, rvargas wrote: > This is most likely not enough. It basically means that the transaction is > moving forward to issue the request (as it stands right now) and when the > response is received we'll assume that the backend has sparse data or can deal > with it. > > I suspect that either we have to go through DoRestartPartialRequest() (and > transition to mode_ = WRITE), or close the entry and transition to pass through > (mode_ = NONE) in case we want to spare the entry. Thanks! Doing DoRestartPartialRequest() is indeed cleaner (I did not know about it before). This will restart the transaction overwriting the entry regardless of the server response, which makes it easier to reason about the effect. I tested this code with this JS code on both Android and Linux: === begin code === var xhr = new XMLHttpRequest(); xhr.open('GET', '/url.html', true); xhr.setRequestHeader('Range', 'bytes=0-1023'); xhr.responseType = 'arraybuffer'; xhr.onreadystatechange = function() { if (xhr.readyState == 4) { document.write("<h1>response1 received</h1>"); var xhr2 = new XMLHttpRequest(); xhr2.open('GET', '/url.html', true); xhr2.setRequestHeader('Range', 'bytes=0-1023'); xhr2.responseType = 'arraybuffer'; xhr2.onreadystatechange = function() { if (xhr2.readyState == 4) { document.write("<h1>response2 received</h1>"); } } xhr2.send(); } } xhr.send(); === end code === The server does not return 206 though. But with the current fix I do not think it is of any interest. I am still wondering how I could test it with a unittest. Modifying the MockDiskEntry to return NOT_IMPLEMENTED seems an overkill. Let me know if you think otherwise or have alternative suggestions.
On 2013/08/27 17:22:13, pasko wrote: > PTAL > > https://codereview.chromium.org/22926031/diff/7001/net/http/http_cache_transa... > File net/http/http_cache_transaction.cc (right): > > https://codereview.chromium.org/22926031/diff/7001/net/http/http_cache_transa... > net/http/http_cache_transaction.cc:1534: // Send the network request if the > backend does not support caching ranges. > On 2013/08/26 15:46:37, gavinp wrote: > > Should we add a TODO here for after the simple cache supports ranges? > > Done. > > https://codereview.chromium.org/22926031/diff/7001/net/http/http_cache_transa... > net/http/http_cache_transaction.cc:1535: next_state_ = STATE_SEND_REQUEST; > On 2013/08/26 21:24:52, rvargas wrote: > > This is most likely not enough. It basically means that the transaction is > > moving forward to issue the request (as it stands right now) and when the > > response is received we'll assume that the backend has sparse data or can deal > > with it. > > > > I suspect that either we have to go through DoRestartPartialRequest() (and > > transition to mode_ = WRITE), or close the entry and transition to pass > through > > (mode_ = NONE) in case we want to spare the entry. > > Thanks! > > Doing DoRestartPartialRequest() is indeed cleaner (I did not know about it > before). This will restart the transaction overwriting the entry regardless of > the server response, which makes it easier to reason about the effect. > > I tested this code with this JS code on both Android and Linux: > === begin code === > var xhr = new XMLHttpRequest(); > xhr.open('GET', '/url.html', true); > xhr.setRequestHeader('Range', 'bytes=0-1023'); > xhr.responseType = 'arraybuffer'; > xhr.onreadystatechange = function() { > if (xhr.readyState == 4) { > document.write("<h1>response1 received</h1>"); > var xhr2 = new XMLHttpRequest(); > xhr2.open('GET', '/url.html', true); > xhr2.setRequestHeader('Range', 'bytes=0-1023'); > xhr2.responseType = 'arraybuffer'; > xhr2.onreadystatechange = function() { > if (xhr2.readyState == 4) { > document.write("<h1>response2 received</h1>"); > } > } > xhr2.send(); > } > } > xhr.send(); > === end code === > > The server does not return 206 though. But with the current fix I do not think > it is of any interest. > > I am still wondering how I could test it with a unittest. Modifying the > MockDiskEntry to return NOT_IMPLEMENTED seems an overkill. Let me know if you > think otherwise or have alternative suggestions. I'm leaning towards allowing the mock entry to return NOT_IMPLEMENTED under direction of a test. The other option would be to instantiate a real simple cache and test against that, but a mock sounds cleaner. We should have a unit test.
lgtm, to remove my earlier anti lgtm. Please get another lgtm from Ricardo on unit testing before landing. Thanks!
On 2013/08/27 21:50:25, rvargas wrote: > > The server does not return 206 though. But with the current fix I do not think > > it is of any interest. > > > > I am still wondering how I could test it with a unittest. Modifying the > > MockDiskEntry to return NOT_IMPLEMENTED seems an overkill. Let me know if you > > think otherwise or have alternative suggestions. > > I'm leaning towards allowing the mock entry to return NOT_IMPLEMENTED under > direction of a test. The other option would be to instantiate a real simple > cache and test against that, but a mock sounds cleaner. We should have a unit > test. I added a test that works by returning ERR_NOT_IMPLEMENTED from the mock. Please take a look. The test passes, however, it does not trigger the code I am fixing, namely, it does not invoke the ReadyForSparseIO, but goes directly to AppendResponseDataToEntry(). I would appreciate a few more hints on how this ~2500 line state machine works. And yes, I don't understand what I am doing :)
On 2013/08/28 19:05:58, pasko wrote: > On 2013/08/27 21:50:25, rvargas wrote: > > > The server does not return 206 though. But with the current fix I do not > think > > > it is of any interest. > > > > > > I am still wondering how I could test it with a unittest. Modifying the > > > MockDiskEntry to return NOT_IMPLEMENTED seems an overkill. Let me know if > you > > > think otherwise or have alternative suggestions. > > > > I'm leaning towards allowing the mock entry to return NOT_IMPLEMENTED under > > direction of a test. The other option would be to instantiate a real simple > > cache and test against that, but a mock sounds cleaner. We should have a unit > > test. > > I added a test that works by returning ERR_NOT_IMPLEMENTED from the mock. Please > take a look. > > The test passes, however, it does not trigger the code I am fixing, namely, it > does not invoke the ReadyForSparseIO, but goes directly to > AppendResponseDataToEntry(). I would appreciate a few more hints on how this > ~2500 line state machine works. And yes, I don't understand what I am doing :) I get more understanding now .. The CL is doing a fallback for STATE_CACHE_QUERY_DATA, which happens as a direct result of STATE_CACHE_READ_RESPONSE with mode READ_WRITE. This probably means that the entry existed and was readable before transaction. Any advice why this happened with clear cache and XHR request would be very helpful. Also, not quite clear how to best emulate it with a test.
On 2013/08/28 19:23:44, pasko wrote: > On 2013/08/28 19:05:58, pasko wrote: > > On 2013/08/27 21:50:25, rvargas wrote: > > > > The server does not return 206 though. But with the current fix I do not > > think > > > > it is of any interest. > > > > > > > > I am still wondering how I could test it with a unittest. Modifying the > > > > MockDiskEntry to return NOT_IMPLEMENTED seems an overkill. Let me know if > > you > > > > think otherwise or have alternative suggestions. > > > > > > I'm leaning towards allowing the mock entry to return NOT_IMPLEMENTED under > > > direction of a test. The other option would be to instantiate a real simple > > > cache and test against that, but a mock sounds cleaner. We should have a > unit > > > test. > > > > I added a test that works by returning ERR_NOT_IMPLEMENTED from the mock. > Please > > take a look. > > > > The test passes, however, it does not trigger the code I am fixing, namely, it > > does not invoke the ReadyForSparseIO, but goes directly to > > AppendResponseDataToEntry(). I would appreciate a few more hints on how this > > ~2500 line state machine works. And yes, I don't understand what I am doing :) > > I get more understanding now .. The CL is doing a fallback for > STATE_CACHE_QUERY_DATA, which happens as a direct result of > STATE_CACHE_READ_RESPONSE with mode READ_WRITE. This probably means that the > entry existed and was readable before transaction. Any advice why this happened > with clear cache and XHR request would be very helpful. Also, not quite clear > how to best emulate it with a test. This made me into thinking. I am also no longer sure whether exactly this situation triggered the problem in media tests. I would need to reproduce the media tests problem from a version a few months ago, and make sure it is not something different. This change would be important to have regardless of the results of those investigations since it fixes a real problem with XHR. I just cannot reproduce it with http_cache_unittests yet.
On 2013/08/28 19:29:04, pasko wrote: > On 2013/08/28 19:23:44, pasko wrote: > > On 2013/08/28 19:05:58, pasko wrote: > > > On 2013/08/27 21:50:25, rvargas wrote: > > > > > The server does not return 206 though. But with the current fix I do not > > > think > > > > > it is of any interest. > > > > > > > > > > I am still wondering how I could test it with a unittest. Modifying the > > > > > MockDiskEntry to return NOT_IMPLEMENTED seems an overkill. Let me know > if > > > you > > > > > think otherwise or have alternative suggestions. > > > > > > > > I'm leaning towards allowing the mock entry to return NOT_IMPLEMENTED > under > > > > direction of a test. The other option would be to instantiate a real > simple > > > > cache and test against that, but a mock sounds cleaner. We should have a > > unit > > > > test. > > > > > > I added a test that works by returning ERR_NOT_IMPLEMENTED from the mock. > > Please > > > take a look. > > > > > > The test passes, however, it does not trigger the code I am fixing, namely, > it > > > does not invoke the ReadyForSparseIO, but goes directly to > > > AppendResponseDataToEntry(). I would appreciate a few more hints on how this > > > ~2500 line state machine works. And yes, I don't understand what I am doing > :) > > > > I get more understanding now .. The CL is doing a fallback for > > STATE_CACHE_QUERY_DATA, which happens as a direct result of > > STATE_CACHE_READ_RESPONSE with mode READ_WRITE. This probably means that the > > entry existed and was readable before transaction. Any advice why this > happened > > with clear cache and XHR request would be very helpful. Also, not quite clear > > how to best emulate it with a test. > > This made me into thinking. I am also no longer sure whether exactly this > situation triggered the problem in media tests. I would need to reproduce the > media tests problem from a version a few months ago, and make sure it is not > something different. This change would be important to have regardless of the > results of those investigations since it fixes a real problem with XHR. I just > cannot reproduce it with http_cache_unittests yet. yeah, sorry about that. This path means that the cache found a regular entry (don't know yet if it is sparse or not), and it is going to inspect it to see if it is sparse or not... and it is doing that because the request specifies a byte range. So in the test, before issuing the byte range request something has to be stored for that url. The flow for this pattern (going through CacheQueryData) is spelled around line 586 of the cc file.
On 2013/08/28 20:04:35, rvargas wrote: > On 2013/08/28 19:29:04, pasko wrote: > > On 2013/08/28 19:23:44, pasko wrote: > > > On 2013/08/28 19:05:58, pasko wrote: > > > > On 2013/08/27 21:50:25, rvargas wrote: > > > > > > The server does not return 206 though. But with the current fix I do > not > > > > think > > > > > > it is of any interest. > > > > > > > > > > > > I am still wondering how I could test it with a unittest. Modifying > the > > > > > > MockDiskEntry to return NOT_IMPLEMENTED seems an overkill. Let me know > > if > > > > you > > > > > > think otherwise or have alternative suggestions. > > > > > > > > > > I'm leaning towards allowing the mock entry to return NOT_IMPLEMENTED > > under > > > > > direction of a test. The other option would be to instantiate a real > > simple > > > > > cache and test against that, but a mock sounds cleaner. We should have a > > > unit > > > > > test. > > > > > > > > I added a test that works by returning ERR_NOT_IMPLEMENTED from the mock. > > > Please > > > > take a look. > > > > > > > > The test passes, however, it does not trigger the code I am fixing, > namely, > > it > > > > does not invoke the ReadyForSparseIO, but goes directly to > > > > AppendResponseDataToEntry(). I would appreciate a few more hints on how > this > > > > ~2500 line state machine works. And yes, I don't understand what I am > doing > > :) > > > > > > I get more understanding now .. The CL is doing a fallback for > > > STATE_CACHE_QUERY_DATA, which happens as a direct result of > > > STATE_CACHE_READ_RESPONSE with mode READ_WRITE. This probably means that the > > > entry existed and was readable before transaction. Any advice why this > > happened > > > with clear cache and XHR request would be very helpful. Also, not quite > clear > > > how to best emulate it with a test. > > > > This made me into thinking. I am also no longer sure whether exactly this > > situation triggered the problem in media tests. I would need to reproduce the > > media tests problem from a version a few months ago, and make sure it is not > > something different. This change would be important to have regardless of the > > results of those investigations since it fixes a real problem with XHR. I just > > cannot reproduce it with http_cache_unittests yet. > > yeah, sorry about that. This path means that the cache found a regular entry > (don't know yet if it is sparse or not), and it is going to inspect it to see if > it is sparse or not... and it is doing that because the request specifies a byte > range. So in the test, before issuing the byte range request something has to be > stored for that url. > > The flow for this pattern (going through CacheQueryData) is spelled around line > 586 of the cc file. OK. Gavin helped me to repro the case: making a MockTransaction was not immediately obvious to me. I uploaded the change, PTAL. One remaining thing not clear to me: whether it is safe to assert on running ReadyForSparseIO()+WriteSparseData() on the 2nd transaction and having only WriteSparseData() on the 3rd.
On 2013/08/28 20:11:52, pasko wrote: > On 2013/08/28 20:04:35, rvargas wrote: > > On 2013/08/28 19:29:04, pasko wrote: > > > On 2013/08/28 19:23:44, pasko wrote: > > > > On 2013/08/28 19:05:58, pasko wrote: > > > > > On 2013/08/27 21:50:25, rvargas wrote: > > > > > > > The server does not return 206 though. But with the current fix I do > > not > > > > > think > > > > > > > it is of any interest. > > > > > > > > > > > > > > I am still wondering how I could test it with a unittest. Modifying > > the > > > > > > > MockDiskEntry to return NOT_IMPLEMENTED seems an overkill. Let me > know > > > if > > > > > you > > > > > > > think otherwise or have alternative suggestions. > > > > > > > > > > > > I'm leaning towards allowing the mock entry to return NOT_IMPLEMENTED > > > under > > > > > > direction of a test. The other option would be to instantiate a real > > > simple > > > > > > cache and test against that, but a mock sounds cleaner. We should have > a > > > > unit > > > > > > test. > > > > > > > > > > I added a test that works by returning ERR_NOT_IMPLEMENTED from the > mock. > > > > Please > > > > > take a look. > > > > > > > > > > The test passes, however, it does not trigger the code I am fixing, > > namely, > > > it > > > > > does not invoke the ReadyForSparseIO, but goes directly to > > > > > AppendResponseDataToEntry(). I would appreciate a few more hints on how > > this > > > > > ~2500 line state machine works. And yes, I don't understand what I am > > doing > > > :) > > > > > > > > I get more understanding now .. The CL is doing a fallback for > > > > STATE_CACHE_QUERY_DATA, which happens as a direct result of > > > > STATE_CACHE_READ_RESPONSE with mode READ_WRITE. This probably means that > the > > > > entry existed and was readable before transaction. Any advice why this > > > happened > > > > with clear cache and XHR request would be very helpful. Also, not quite > > clear > > > > how to best emulate it with a test. > > > > > > This made me into thinking. I am also no longer sure whether exactly this > > > situation triggered the problem in media tests. I would need to reproduce > the > > > media tests problem from a version a few months ago, and make sure it is not > > > something different. This change would be important to have regardless of > the > > > results of those investigations since it fixes a real problem with XHR. I > just > > > cannot reproduce it with http_cache_unittests yet. > > > > yeah, sorry about that. This path means that the cache found a regular entry > > (don't know yet if it is sparse or not), and it is going to inspect it to see > if > > it is sparse or not... and it is doing that because the request specifies a > byte > > range. So in the test, before issuing the byte range request something has to > be > > stored for that url. > > > > The flow for this pattern (going through CacheQueryData) is spelled around > line > > 586 of the cc file. > > OK. Gavin helped me to repro the case: making a MockTransaction was not > immediately obvious to me. I uploaded the change, PTAL. > > One remaining thing not clear to me: whether it is safe to assert on running > ReadyForSparseIO()+WriteSparseData() on the 2nd transaction and having only > WriteSparseData() on the 3rd. I think we need two specific tests: range request after 200 and range request with an empty cache. The first one should delete the entry (verified either by another tx that should not open any entry, or by directly querying the cache). The second test should verify that the final state of the entry makes sense. If we delete the entry, then the check is the same as the first test. If we don't delete the entry, we may need to look at the cache flags directly as opposed to attempting another transaction (see HttpCache.WriteResponseInfo_Truncated)... I think we want to delete the entry though.
https://codereview.chromium.org/22926031/diff/42001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/22926031/diff/42001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:499: #if defined(OS_ANDROID) There should not be an ifdef here. https://codereview.chromium.org/22926031/diff/42001/net/http/http_cache_unitt... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/22926031/diff/42001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:3429: RunTransactionTestWithResponse(cache.http_cache(), kRangeGET_NotReallyARange, I would actually prefer following HttpCache.RangeGET_Previous200 https://codereview.chromium.org/22926031/diff/42001/net/http/mock_http_cache.cc File net/http/mock_http_cache.cc (right): https://codereview.chromium.org/22926031/diff/42001/net/http/mock_http_cache.... net/http/mock_http_cache.cc:138: backend_->NotifySparseIO(); This is slightly problematic because the contract doesn't require entries to be closed before the cache (true for the backend and the http cache/transaction). Do we really need the count?
https://codereview.chromium.org/22926031/diff/42001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/22926031/diff/42001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:499: #if defined(OS_ANDROID) On 2013/08/28 21:35:47, rvargas wrote: > There should not be an ifdef here. This was specifically requested by the TPM to make this CL mergeable. Perhaps the thing to do is add a TODO() to remove this #ifdef after the change has baked on canary for a day and then been merged?
I'll take a look tomorrow on splitting the test into two. I have a proposal to have the test cleanup in a separate CL. The proposal can be rejected on valid grounds, of course, in case we are still not sure the code does what we want. https://codereview.chromium.org/22926031/diff/42001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/22926031/diff/42001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:499: #if defined(OS_ANDROID) On 2013/08/28 21:41:25, gavinp wrote: > On 2013/08/28 21:35:47, rvargas wrote: > > There should not be an ifdef here. > > This was specifically requested by the TPM to make this CL mergeable. Perhaps > the thing to do is add a TODO() to remove this #ifdef after the change has baked > on canary for a day and then been merged? I don't like this ifdef either, and maybe this one is a bit of extreme, just wanted the TPM to feel cozy. I left a TODO in http_cache_transaction.cc to remove this conditionalizing, and it then did not make much sense to replicate that TODO at every hunk in the patch. https://codereview.chromium.org/22926031/diff/42001/net/http/http_cache_unitt... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/22926031/diff/42001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:3429: RunTransactionTestWithResponse(cache.http_cache(), kRangeGET_NotReallyARange, On 2013/08/28 21:35:47, rvargas wrote: > I would actually prefer following HttpCache.RangeGET_Previous200 Done. https://codereview.chromium.org/22926031/diff/42001/net/http/mock_http_cache.cc File net/http/mock_http_cache.cc (right): https://codereview.chromium.org/22926031/diff/42001/net/http/mock_http_cache.... net/http/mock_http_cache.cc:138: backend_->NotifySparseIO(); On 2013/08/28 21:35:47, rvargas wrote: > This is slightly problematic because the contract doesn't require entries to be > closed before the cache (true for the backend and the http cache/transaction). I did not quite understand this sentence, can you clarify? > Do we really need the count? With checking entry state manually it may be not necessary. I'll take a look tomorrow. Altogether having a count sounds consistent with the practices in these mocks.
https://codereview.chromium.org/22926031/diff/42001/net/http/http_cache_unitt... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/22926031/diff/42001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:3429: RunTransactionTestWithResponse(cache.http_cache(), kRangeGET_NotReallyARange, On 2013/08/28 21:56:28, pasko wrote: > On 2013/08/28 21:35:47, rvargas wrote: > > I would actually prefer following HttpCache.RangeGET_Previous200 > > Done. huh, misunderstood you here. I thought you suggested renaming, but you probably suggested following the pattern of the HttpCache.RangeGET_Previous200 test. This test is a bit longer though :/ What is the benefit?
https://codereview.chromium.org/22926031/diff/42001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/22926031/diff/42001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:499: #if defined(OS_ANDROID) On 2013/08/28 21:56:28, pasko wrote: > On 2013/08/28 21:41:25, gavinp wrote: > > On 2013/08/28 21:35:47, rvargas wrote: > > > There should not be an ifdef here. > > > > This was specifically requested by the TPM to make this CL mergeable. Perhaps > > the thing to do is add a TODO() to remove this #ifdef after the change has > baked > > on canary for a day and then been merged? > > I don't like this ifdef either, and maybe this one is a bit of extreme, just > wanted the TPM to feel cozy. I left a TODO in http_cache_transaction.cc to > remove this conditionalizing, and it then did not make much sense to replicate > that TODO at every hunk in the patch. I just commented on the bug. I understand the other ifdef... this one is out of place IMO. (but don't take this comment as blocking the CL in any way, because it doesn't break anything) https://codereview.chromium.org/22926031/diff/42001/net/http/http_cache_unitt... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/22926031/diff/42001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:3429: RunTransactionTestWithResponse(cache.http_cache(), kRangeGET_NotReallyARange, On 2013/08/28 22:01:43, pasko wrote: > On 2013/08/28 21:56:28, pasko wrote: > > On 2013/08/28 21:35:47, rvargas wrote: > > > I would actually prefer following HttpCache.RangeGET_Previous200 > > > > Done. > > huh, misunderstood you here. I thought you suggested renaming, but you probably > suggested following the pattern of the HttpCache.RangeGET_Previous200 test. > > This test is a bit longer though :/ What is the benefit? Not really following the whole pattern of that test, just the few lines that prime the cache. It has two advantages: - By reading the test it is pretty clear how the two requests differ. I don't have to go somewhere else to see if the URL is the same, or any other field of the structure. - It makes sure that the cached data is usable to complete the byte-range-request as is. Using this specific mock transaction may actually fail by itself because it cannot be used to fulfill a request for bytes 40-49. Sure, the test actually works because we detect the error code before looking at the cached data, but that is not clear from the point of view of the test. Just so that there's no confusion, what I was proposing was one test that: -primes the cache -issues the test tx -verifies the entry is gone (another tx?) and another that skips the first step. https://codereview.chromium.org/22926031/diff/42001/net/http/mock_http_cache.cc File net/http/mock_http_cache.cc (right): https://codereview.chromium.org/22926031/diff/42001/net/http/mock_http_cache.... net/http/mock_http_cache.cc:138: backend_->NotifySparseIO(); On 2013/08/28 21:56:28, pasko wrote: > On 2013/08/28 21:35:47, rvargas wrote: > > This is slightly problematic because the contract doesn't require entries to > be > > closed before the cache (true for the backend and the http cache/transaction). > > I did not quite understand this sentence, can you clarify? The thing is that it is possible (valid, and used) to keep working with an HttpCache::Transaction and with a disk_cache::Entry after the HttpCache (and backend) goes away. That doesn't mean that things are going to work fine, it means that things should not crash... keeping this pointer here means we could crash. We can fix this behavior, but it seems simpler just to not have a count in the first place, especially given that the whole thing is supposed to be somewhat short lived. > > > Do we really need the count? > > With checking entry state manually it may be not necessary. I'll take a look > tomorrow. Altogether having a count sounds consistent with the practices in > these mocks. The count makes the test more specific, that's true. But it is not really a bad test without the count (as opposed to other counters on the mocks, which are fundamental to most tests)
On 2013/08/28 21:56:28, pasko wrote: > I'll take a look tomorrow on splitting the test into two. I have a proposal to > have the test cleanup in a separate CL. The proposal can be rejected on valid > grounds, of course, in case we are still not sure the code does what we want. I'm fine with checking in the code without the test if we need to land it asap. I'm confident that we want this part :) We may have to do some extra change to the code when landing the tests, because I have not verified that we don't preserve a failed byte range request, so I'm less confident there. > > https://codereview.chromium.org/22926031/diff/42001/net/disk_cache/simple/sim... > File net/disk_cache/simple/simple_entry_impl.cc (right): > > https://codereview.chromium.org/22926031/diff/42001/net/disk_cache/simple/sim... > net/disk_cache/simple/simple_entry_impl.cc:499: #if defined(OS_ANDROID) > On 2013/08/28 21:41:25, gavinp wrote: > > On 2013/08/28 21:35:47, rvargas wrote: > > > There should not be an ifdef here. > > > > This was specifically requested by the TPM to make this CL mergeable. Perhaps > > the thing to do is add a TODO() to remove this #ifdef after the change has > baked > > on canary for a day and then been merged? > > I don't like this ifdef either, and maybe this one is a bit of extreme, just > wanted the TPM to feel cozy. I left a TODO in http_cache_transaction.cc to > remove this conditionalizing, and it then did not make much sense to replicate > that TODO at every hunk in the patch. > > https://codereview.chromium.org/22926031/diff/42001/net/http/http_cache_unitt... > File net/http/http_cache_unittest.cc (right): > > https://codereview.chromium.org/22926031/diff/42001/net/http/http_cache_unitt... > net/http/http_cache_unittest.cc:3429: > RunTransactionTestWithResponse(cache.http_cache(), kRangeGET_NotReallyARange, > On 2013/08/28 21:35:47, rvargas wrote: > > I would actually prefer following HttpCache.RangeGET_Previous200 > > Done. > > https://codereview.chromium.org/22926031/diff/42001/net/http/mock_http_cache.cc > File net/http/mock_http_cache.cc (right): > > https://codereview.chromium.org/22926031/diff/42001/net/http/mock_http_cache.... > net/http/mock_http_cache.cc:138: backend_->NotifySparseIO(); > On 2013/08/28 21:35:47, rvargas wrote: > > This is slightly problematic because the contract doesn't require entries to > be > > closed before the cache (true for the backend and the http cache/transaction). > > I did not quite understand this sentence, can you clarify? > > > Do we really need the count? > > With checking entry state manually it may be not necessary. I'll take a look > tomorrow. Altogether having a count sounds consistent with the practices in > these mocks.
I wrote two tests to closely reflect Ricardo's recommendations. After a few hours debugging internals of the testing infrastructure it is finally passing. PTAL. Фуф.
lgtm
Thankg Ricardo and Gavin for review and numerous hints on testing.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pasko@chromium.org/22926031/90001
Message was sent while issue was closed.
Change committed as 220564 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
