|
|
Created:
3 years, 7 months ago by shivanisha Modified:
3 years, 5 months ago CC:
cbentzel+watch_chromium.org, chromium-reviews, gavinp+disk_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis CL adds a new class HttpCache::Writers which will implement multiple cache
transactions reading from the network and writing to the cache enabling each of
those transactions to drive reading the response body from the network. This
ensures that a slow consumer does not starve other consumers of the same resource.
This CL implements the Writers class in isolation and does not create an
instance or integrates it with HttpCache/HttpCache::Transaction. In a follow up CL
which will be dependent on parallel validation CL
(https://codereview.chromium.org/2721933002/), a Writer object will replace entry-
>writer. In case of transaction types that do not support shared writing like
partial transactions, Writers will only contain one transaction at a time and will
have is_exclusive set to true.
The flow of Read() call will be:
URLRequestHttpJob -> HttpCache::Transaction::Read() -> HttpCache::Writers::Read()
-> HttpNetworkTransaction::Read()
HttpCache::Writers will invoke HC::T's callback after writing the read buffer to
the cache successfully or on network read failure/ cache write failure.
So, in essence, HttpCache::Writers::Read() abstracts away the DoNetworkRead* and
DoCacheWriteData* functions of HC::T
HttpCache::Transaction that has invoked Read() is the |active_transaction_| and
may be killed at any time by it's consumer.
When a transaction completes writing to the cache, either failure or success, it
invokes HttpCache::DoneWritingToEntry as it does now which will invoke the
required functionality in Writers.
But If |active_transaction_| does not exist, Writers object invokes
HttpCache::DoneWritingToEntry itself.
BUG=472740
Review-Url: https://codereview.chromium.org/2886483002
Cr-Commit-Position: refs/heads/master@{#486907}
Committed: https://chromium.googlesource.com/chromium/src/+/c6582e1924c76f68e2b8873712cdf26a0b0c1edb
Patch Set 1 : Initial patch #Patch Set 2 : copyright + include + lint suggestions #Patch Set 3 : Unit tests added + minor changes #
Total comments: 24
Patch Set 4 : rdsmith feedback #
Total comments: 1
Patch Set 5 : Windows compile error fixed with NET_EXPORT_PRIVATE #Patch Set 6 : AddTransaction to take unique_ptr of network transaction. #
Total comments: 92
Patch Set 7 : Feedback addressed #Patch Set 8 : Feedback addressed #
Total comments: 4
Patch Set 9 : rdsmith feedback #Patch Set 10 : Rebased till refs/heads/master@{#475981} #
Total comments: 50
Patch Set 11 : Feedback addressed #
Total comments: 52
Patch Set 12 : rdsmith feedback addressed #Patch Set 13 : Addressed all feedback (includes new tests) #Patch Set 14 : Rebased with refs/heads/master@{#479258} #
Total comments: 32
Patch Set 15 : Feedback addressed #
Total comments: 12
Patch Set 16 : Rebased with refs/heads/master@{#482382} #Patch Set 17 : Tests feedback addressed #Patch Set 18 : Win compile errors fixed #Patch Set 19 : Converted test class into a test fixture class. #Patch Set 20 : Fix test class memory leak #
Total comments: 110
Patch Set 21 : Test memory leak + rdsmith feedback addressed #Patch Set 22 : jkarlin feebdack addressed #
Total comments: 12
Patch Set 23 : rdsmith@ feedback addressed #Patch Set 24 : jkarlin@ feedback addressed. #
Total comments: 26
Patch Set 25 : Rebased with refs/heads/master@{#484325} #Patch Set 26 : Added MockHttpCacheTransaction #Patch Set 27 : Feedback addressed. #
Total comments: 36
Patch Set 28 : Windows compile error fixed. #Patch Set 29 : Feedback addressed #
Total comments: 2
Patch Set 30 : Rebased with refs/heads/master@{#486443} #Patch Set 31 : Comment changed. #Messages
Total messages: 156 (99 generated)
Description was changed from ========== This CL adds a new class HttpCache::Writers which will implement multiple cache transactions reading from the network and writing to the cache enabling each of those transactions to drive reading the response body from the network. This ensures that a slow consumer does not starve other consumers of the same resource. This CL only implements the Writers class in isolation and does not create an instance. In a follow up CL which will be dependent on parallel validation CL (https://codereview.chromium.org/2721933002/), a Writer object will replace entry->writer. In case of transaction types that do not support shared writing like partial transactions, Writers can only contain one transaction at a time and will have is_exclusive set to true. The flow of Read() call will be: URLRequestHttpJob -> HttpCache::Transaction::Read() -> HttpCache::Writers::Read() -> HttpNetworkTransaction::Read() HttpCache::Writers will invoke HC::T's callback after writing the read buffer to the cache successfully or on network read failure/ cache write failure. So, in essence, writers HttpCache::Writers::Read() abstracts away the DoNetworkRead* and DoCacheWriteData* functions of HC::T HttpCache::Transaction that has invoked Read(), |active_transaction_| may be killed at any time by it's consumer. When a transaction completes writing to the cache, either failure or success, it invokes HttpCache::DoneWritingToEntry as it does now which will invoke the required functionality in Writers. If |active_transaction_| does not exist, Writers object invokes HttpCache::DoneWritingToEntry itself. Entry related functionality like WriteResponseInfo and IsResponseCompleted is added to HttpCache since HC::T may not be alive. Later, this functionality will be removed from HC::T completely. Tests will be added in the CL that integrates this class with the HttpCache and HttpCache::Transaction layers. BUG=472740 ========== to ========== This CL adds a new class HttpCache::Writers which will implement multiple cache transactions reading from the network and writing to the cache enabling each of those transactions to drive reading the response body from the network. This ensures that a slow consumer does not starve other consumers of the same resource. This CL only implements the Writers class in isolation and does not create an instance or integrates it with HttpCache/HttpCache::Transaction. In a follow up CL which will be dependent on parallel validation CL (https://codereview.chromium.org/2721933002/), a Writer object will replace entry->writer. In case of transaction types that do not support shared writing like partial transactions, Writers will only contain one transaction at a time and will have is_exclusive set to true. The flow of Read() call will be: URLRequestHttpJob -> HttpCache::Transaction::Read() -> HttpCache::Writers::Read() -> HttpNetworkTransaction::Read() HttpCache::Writers will invoke HC::T's callback after writing the read buffer to the cache successfully or on network read failure/ cache write failure. So, in essence, HttpCache::Writers::Read() abstracts away the DoNetworkRead* and DoCacheWriteData* functions of HC::T HttpCache::Transaction that has invoked Read() is the |active_transaction_| and may be killed at any time by it's consumer. When a transaction completes writing to the cache, either failure or success, it invokes HttpCache::DoneWritingToEntry as it does now which will invoke the required functionality in Writers. But If |active_transaction_| does not exist, Writers object invokes HttpCache::DoneWritingToEntry itself. Tests will be added in the CL that integrates this class with the HttpCache and HttpCache::Transaction layers. BUG=472740 ==========
Description was changed from ========== This CL adds a new class HttpCache::Writers which will implement multiple cache transactions reading from the network and writing to the cache enabling each of those transactions to drive reading the response body from the network. This ensures that a slow consumer does not starve other consumers of the same resource. This CL only implements the Writers class in isolation and does not create an instance or integrates it with HttpCache/HttpCache::Transaction. In a follow up CL which will be dependent on parallel validation CL (https://codereview.chromium.org/2721933002/), a Writer object will replace entry->writer. In case of transaction types that do not support shared writing like partial transactions, Writers will only contain one transaction at a time and will have is_exclusive set to true. The flow of Read() call will be: URLRequestHttpJob -> HttpCache::Transaction::Read() -> HttpCache::Writers::Read() -> HttpNetworkTransaction::Read() HttpCache::Writers will invoke HC::T's callback after writing the read buffer to the cache successfully or on network read failure/ cache write failure. So, in essence, HttpCache::Writers::Read() abstracts away the DoNetworkRead* and DoCacheWriteData* functions of HC::T HttpCache::Transaction that has invoked Read() is the |active_transaction_| and may be killed at any time by it's consumer. When a transaction completes writing to the cache, either failure or success, it invokes HttpCache::DoneWritingToEntry as it does now which will invoke the required functionality in Writers. But If |active_transaction_| does not exist, Writers object invokes HttpCache::DoneWritingToEntry itself. Tests will be added in the CL that integrates this class with the HttpCache and HttpCache::Transaction layers. BUG=472740 ========== to ========== This CL adds a new class HttpCache::Writers which will implement multiple cache transactions reading from the network and writing to the cache enabling each of those transactions to drive reading the response body from the network. This ensures that a slow consumer does not starve other consumers of the same resource. This CL only implements the Writers class in isolation and does not create an instance or integrates it with HttpCache/HttpCache::Transaction. In a follow up CL which will be dependent on parallel validation CL (https://codereview.chromium.org/2721933002/), a Writer object will replace entry- >writer. In case of transaction types that do not support shared writing like partial transactions, Writers will only contain one transaction at a time and will have is_exclusive set to true. The flow of Read() call will be: URLRequestHttpJob -> HttpCache::Transaction::Read() -> HttpCache::Writers::Read() -> HttpNetworkTransaction::Read() HttpCache::Writers will invoke HC::T's callback after writing the read buffer to the cache successfully or on network read failure/ cache write failure. So, in essence, HttpCache::Writers::Read() abstracts away the DoNetworkRead* and DoCacheWriteData* functions of HC::T HttpCache::Transaction that has invoked Read() is the |active_transaction_| and may be killed at any time by it's consumer. When a transaction completes writing to the cache, either failure or success, it invokes HttpCache::DoneWritingToEntry as it does now which will invoke the required functionality in Writers. But If |active_transaction_| does not exist, Writers object invokes HttpCache::DoneWritingToEntry itself. Tests will be added in the CL that integrates this class with the HttpCache and HttpCache::Transaction layers. BUG=472740 ==========
Description was changed from ========== This CL adds a new class HttpCache::Writers which will implement multiple cache transactions reading from the network and writing to the cache enabling each of those transactions to drive reading the response body from the network. This ensures that a slow consumer does not starve other consumers of the same resource. This CL only implements the Writers class in isolation and does not create an instance or integrates it with HttpCache/HttpCache::Transaction. In a follow up CL which will be dependent on parallel validation CL (https://codereview.chromium.org/2721933002/), a Writer object will replace entry- >writer. In case of transaction types that do not support shared writing like partial transactions, Writers will only contain one transaction at a time and will have is_exclusive set to true. The flow of Read() call will be: URLRequestHttpJob -> HttpCache::Transaction::Read() -> HttpCache::Writers::Read() -> HttpNetworkTransaction::Read() HttpCache::Writers will invoke HC::T's callback after writing the read buffer to the cache successfully or on network read failure/ cache write failure. So, in essence, HttpCache::Writers::Read() abstracts away the DoNetworkRead* and DoCacheWriteData* functions of HC::T HttpCache::Transaction that has invoked Read() is the |active_transaction_| and may be killed at any time by it's consumer. When a transaction completes writing to the cache, either failure or success, it invokes HttpCache::DoneWritingToEntry as it does now which will invoke the required functionality in Writers. But If |active_transaction_| does not exist, Writers object invokes HttpCache::DoneWritingToEntry itself. Tests will be added in the CL that integrates this class with the HttpCache and HttpCache::Transaction layers. BUG=472740 ========== to ========== This CL adds a new class HttpCache::Writers which will implement multiple cache transactions reading from the network and writing to the cache enabling each of those transactions to drive reading the response body from the network. This ensures that a slow consumer does not starve other consumers of the same resource. This CL implements the Writers class in isolation and does not create an instance or integrates it with HttpCache/HttpCache::Transaction. In a follow up CL which will be dependent on parallel validation CL (https://codereview.chromium.org/2721933002/), a Writer object will replace entry- >writer. In case of transaction types that do not support shared writing like partial transactions, Writers will only contain one transaction at a time and will have is_exclusive set to true. The flow of Read() call will be: URLRequestHttpJob -> HttpCache::Transaction::Read() -> HttpCache::Writers::Read() -> HttpNetworkTransaction::Read() HttpCache::Writers will invoke HC::T's callback after writing the read buffer to the cache successfully or on network read failure/ cache write failure. So, in essence, HttpCache::Writers::Read() abstracts away the DoNetworkRead* and DoCacheWriteData* functions of HC::T HttpCache::Transaction that has invoked Read() is the |active_transaction_| and may be killed at any time by it's consumer. When a transaction completes writing to the cache, either failure or success, it invokes HttpCache::DoneWritingToEntry as it does now which will invoke the required functionality in Writers. But If |active_transaction_| does not exist, Writers object invokes HttpCache::DoneWritingToEntry itself. Tests will be added in the CL that integrates this class with the HttpCache and HttpCache::Transaction layers. BUG=472740 ==========
shivanisha@chromium.org changed reviewers: + jkarlin@chromium.org, rdsmith@chromium.org
Patchset #1 (id:1) has been deleted
Josh, Randy, PTAL, Thanks! This CL adds a new class HttpCache::Writers which will implement multiple cache transactions reading from the network and writing to the cache. It implements the Writers class in isolation and does not create an instance or integrates it with HttpCache/HttpCache::Transaction. In a follow up CL which will be dependent on parallel validation CL (https://codereview.chromium.org/2721933002/), integration and tests will be implemented. (Note: Goma builds since morning is taking forever to build so there may be some compile time errors. If any, I'll fix them in the next patch.)
On 2017/05/15 17:25:02, shivanisha wrote: > Josh, Randy, PTAL, Thanks! > > This CL adds a new class HttpCache::Writers which will implement multiple cache > transactions reading from the network and writing to the cache. It implements > the Writers class in isolation and does not create an instance or integrates it > with HttpCache/HttpCache::Transaction. > In a follow up CL which will be dependent on parallel validation CL > (https://codereview.chromium.org/2721933002/), integration and tests will be > implemented. > > (Note: Goma builds since morning is taking forever to build so there may be some > compile time errors. If any, I'll fix them in the next patch.) Seems like this would be the right cl for a http_cache_writers_unittest.cc file testing Writers.
Patchset #1 (id:20001) has been deleted
On 2017/05/15 at 17:44:11, jkarlin wrote: > On 2017/05/15 17:25:02, shivanisha wrote: > > Josh, Randy, PTAL, Thanks! > > > > This CL adds a new class HttpCache::Writers which will implement multiple cache > > transactions reading from the network and writing to the cache. It implements > > the Writers class in isolation and does not create an instance or integrates it > > with HttpCache/HttpCache::Transaction. > > In a follow up CL which will be dependent on parallel validation CL > > (https://codereview.chromium.org/2721933002/), integration and tests will be > > implemented. > > > > (Note: Goma builds since morning is taking forever to build so there may be some > > compile time errors. If any, I'll fix them in the next patch.) > > Seems like this would be the right cl for a http_cache_writers_unittest.cc file testing Writers. Good suggestion, I initially planned to test it via the HttpCache::Transaction layer after integration but on second thoughts, it might be cleaner to test it individually.
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Patchset #3 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, Thanks! The latest patch includes unit tests. Tests cover the independent aspects of this class like membership functions, reading by a single transaction , stopCaching, truncateEntry etc. Not covering simultaneous reading by multiple transactions in this CL (and concepts related to it) since that has tightly integrated concepts in HC::T and ActiveEntry layers and would be better tested with the those layers integrated. similarly for failure of network read or cache write. Some of the issues that I faced while writing those tests were assertions in DoneWritingToEntry/DoneWithEntry when a transaction is removed since entry does not see its correct state as it belongs to Writers.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Initial comments, primarily focussed on the interface contract. https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:18: // network and write to the cache, depending on its read offset. I'm having a hard time understanding this comment in context. It doesn't make any explicit reference to the class being defined in this file, and hence could easily lead someone who doesn't know the implementation well (who should pretty much always be the target audience) to not understand its relevance to the code below. Maybe something like "If multiple HttpCache::Transactions are accessing the same cache entry, they multiplex their access to that entry through HttpCache::Writers. Any of those transactions may access that entry, either reading from the cache or reading from the network and writing to the cache, depending on the read offset. This ensures that a slow consumer does not starve other consumers of the same resource."? But I'm not committed to the wording, I just want something that uses the vocabulary that a person might understand when they first open this file. https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:30: Writers(base::WeakPtr<HttpCache> cache, ActiveEntry* entry); Two questions: a) How can this transfer a network transaction to Writers if there's no network transaction argument? b) If the Writers object is owned by the ActiveEntry, which is owned by the cache, why is a WeakPtr<> needed? https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:37: // via the |callback|. Given that the offset in the object was mentioned in an earlier comment, it's a bit confusing that this comment implies that everyone shares an offset. Can you update comments somewhere to clarify this? (Either the comment at the top that mentions offset, here, or some additional comment that describes how transactions with different offsets are handled.) https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:50: // Adds a transaction to Writers. Should only be invoked if nit: I believe this only adds HttpCache::Transactions. Given that the signature refers to both those and HttpTransactions, I'd advise being specific about which type you mean. https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:53: // should definitely pass a non-null network transaction. It is ok for This seems contradictory with the constructor comment. https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:54: // |transaction| to die before this object. Leaving the object with an invalid raw pointer? That seems an invitation for use-after-free errors. It would be good if there was a mechanism for cleaning up the Writers object correctly when a transaction dies. (If it's a requirement for the HC::T to call RemoveTransaction when it dies, the right way to write this comment is "|transaction| is required to call |RemoveTransaction(transaction)| before being destroyed." In general, documentation comments should describe what consumers have to do to correct use the interface. (Abstract base class comments are a bit tricky since they also need to describe what producers have to do to satisfy the interface.)) https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:80: // Set exclusive as true. This doesn't actually describe the behavior that the consumer sees; all it says is that a bit is being set on the structure (which is the implementation). What's the change in behavior from setting exclusive to true? https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:84: bool CanAddWriters(); Is this a const function? More generally, for state machine classes, I think of state changing functions as having restrictions on when they can be called (so as to not confuse the state machine). So I have a urge to see everything in this class either having documented restrictions on when they can be called or be marked const. That's obviously not appropriate globally (e.g. Read() handles this just fine internally, either queueing or starting the state machine) but if you could put a moment of those into whether any of the non-const functions in this header should have restrictions on when they can be called I'd be grateful. https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:93: void TruncateEntry(); The fact that the implementation of this involves setting a state and calling into DoLoop() suggests to me that three are specific restrictions on when this can be called (e.g. not when another read is outstanding?). Could you document those conditions, or respond to this comment telling me why it's ok to just do a state transition by fiat at any point in the state machine to a new state? (I'd also value understanding better what the use cases are for uses of this function. That may not be appropriate in the documentation comment--the documentation comment should indicate what the effect of the function is from the consumers POV along with any restrictions the consumer needs to obey in calling it--but if not, respond to this comment with a sketch?) https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:97: // For testing. nit: Test functions usually have "ForTesting" suffixed; I think there's some auto-analysis to confirm they're only used for testing. https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:163: RequestPriority getCurrentHighestPriority(); nit: I don't think this format is allowed by the style guide; it's either lower case separated by underscores (and usually implemented inline) or starting with a capital.
Description was changed from ========== This CL adds a new class HttpCache::Writers which will implement multiple cache transactions reading from the network and writing to the cache enabling each of those transactions to drive reading the response body from the network. This ensures that a slow consumer does not starve other consumers of the same resource. This CL implements the Writers class in isolation and does not create an instance or integrates it with HttpCache/HttpCache::Transaction. In a follow up CL which will be dependent on parallel validation CL (https://codereview.chromium.org/2721933002/), a Writer object will replace entry- >writer. In case of transaction types that do not support shared writing like partial transactions, Writers will only contain one transaction at a time and will have is_exclusive set to true. The flow of Read() call will be: URLRequestHttpJob -> HttpCache::Transaction::Read() -> HttpCache::Writers::Read() -> HttpNetworkTransaction::Read() HttpCache::Writers will invoke HC::T's callback after writing the read buffer to the cache successfully or on network read failure/ cache write failure. So, in essence, HttpCache::Writers::Read() abstracts away the DoNetworkRead* and DoCacheWriteData* functions of HC::T HttpCache::Transaction that has invoked Read() is the |active_transaction_| and may be killed at any time by it's consumer. When a transaction completes writing to the cache, either failure or success, it invokes HttpCache::DoneWritingToEntry as it does now which will invoke the required functionality in Writers. But If |active_transaction_| does not exist, Writers object invokes HttpCache::DoneWritingToEntry itself. Tests will be added in the CL that integrates this class with the HttpCache and HttpCache::Transaction layers. BUG=472740 ========== to ========== This CL adds a new class HttpCache::Writers which will implement multiple cache transactions reading from the network and writing to the cache enabling each of those transactions to drive reading the response body from the network. This ensures that a slow consumer does not starve other consumers of the same resource. This CL implements the Writers class in isolation and does not create an instance or integrates it with HttpCache/HttpCache::Transaction. In a follow up CL which will be dependent on parallel validation CL (https://codereview.chromium.org/2721933002/), a Writer object will replace entry- >writer. In case of transaction types that do not support shared writing like partial transactions, Writers will only contain one transaction at a time and will have is_exclusive set to true. The flow of Read() call will be: URLRequestHttpJob -> HttpCache::Transaction::Read() -> HttpCache::Writers::Read() -> HttpNetworkTransaction::Read() HttpCache::Writers will invoke HC::T's callback after writing the read buffer to the cache successfully or on network read failure/ cache write failure. So, in essence, HttpCache::Writers::Read() abstracts away the DoNetworkRead* and DoCacheWriteData* functions of HC::T HttpCache::Transaction that has invoked Read() is the |active_transaction_| and may be killed at any time by it's consumer. When a transaction completes writing to the cache, either failure or success, it invokes HttpCache::DoneWritingToEntry as it does now which will invoke the required functionality in Writers. But If |active_transaction_| does not exist, Writers object invokes HttpCache::DoneWritingToEntry itself. BUG=472740 ==========
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, Thanks! The latest patch has Randy's feedback addressed. https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:18: // network and write to the cache, depending on its read offset. On 2017/05/16 at 22:35:28, Randy Smith (Not in Mondays) wrote: > I'm having a hard time understanding this comment in context. It doesn't make any explicit reference to the class being defined in this file, and hence could easily lead someone who doesn't know the implementation well (who should pretty much always be the target audience) to not understand its relevance to the code below. > > Maybe something like "If multiple HttpCache::Transactions are accessing the same cache entry, they multiplex their access to that entry through HttpCache::Writers. Any of those transactions may access that entry, either reading from the cache or reading from the network and writing to the cache, depending on the read offset. This ensures that a slow consumer does not starve other consumers of the same resource."? But I'm not committed to the wording, I just want something that uses the vocabulary that a person might understand when they first open this file. Sounds good. Updated the comment. https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:30: Writers(base::WeakPtr<HttpCache> cache, ActiveEntry* entry); On 2017/05/16 at 22:35:29, Randy Smith (Not in Mondays) wrote: > Two questions: > a) How can this transfer a network transaction to Writers if there's no network transaction argument? > b) If the Writers object is owned by the ActiveEntry, which is owned by the cache, why is a WeakPtr<> needed? a) Removed that part of the comment. Was a remnant from an earlier iteration. Thanks b) Converted to a raw pointer https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:37: // via the |callback|. On 2017/05/16 at 22:35:29, Randy Smith (Not in Mondays) wrote: > Given that the offset in the object was mentioned in an earlier comment, it's a bit confusing that this comment implies that everyone shares an offset. Can you update comments somewhere to clarify this? (Either the comment at the top that mentions offset, here, or some additional comment that describes how transactions with different offsets are handled.) Removed the reference to offset in the above comment. Transactions invoking Read here will always be at the latest offset else they would not invoke Read and will read the cached data in the HC::T state machine itself. https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:50: // Adds a transaction to Writers. Should only be invoked if On 2017/05/16 at 22:35:28, Randy Smith (Not in Mondays) wrote: > nit: I believe this only adds HttpCache::Transactions. Given that the signature refers to both those and HttpTransactions, I'd advise being specific about which type you mean. Done https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:53: // should definitely pass a non-null network transaction. It is ok for On 2017/05/16 at 22:35:28, Randy Smith (Not in Mondays) wrote: > This seems contradictory with the constructor comment. Constructor comment corrected. https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:54: // |transaction| to die before this object. On 2017/05/16 at 22:35:29, Randy Smith (Not in Mondays) wrote: > Leaving the object with an invalid raw pointer? That seems an invitation for use-after-free errors. It would be good if there was a mechanism for cleaning up the Writers object correctly when a transaction dies. > > (If it's a requirement for the HC::T to call RemoveTransaction when it dies, the right way to write this comment is "|transaction| is required to call |RemoveTransaction(transaction)| before being destroyed." In general, documentation comments should describe what consumers have to do to correct use the interface. (Abstract base class comments are a bit tricky since they also need to describe what producers have to do to satisfy the interface.)) Updated the comment to include RemoveTransaction requirement. Thanks https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:80: // Set exclusive as true. On 2017/05/16 at 22:35:29, Randy Smith (Not in Mondays) wrote: > This doesn't actually describe the behavior that the consumer sees; all it says is that a bit is being set on the structure (which is the implementation). What's the change in behavior from setting exclusive to true? Done https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:84: bool CanAddWriters(); On 2017/05/16 at 22:35:29, Randy Smith (Not in Mondays) wrote: > Is this a const function? > > More generally, for state machine classes, I think of state changing functions as having restrictions on when they can be called (so as to not confuse the state machine). So I have a urge to see everything in this class either having documented restrictions on when they can be called or be marked const. That's obviously not appropriate globally (e.g. Read() handles this just fine internally, either queueing or starting the state machine) but if you could put a moment of those into whether any of the non-const functions in this header should have restrictions on when they can be called I'd be grateful. Made a few functions as const. Thanks https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:93: void TruncateEntry(); On 2017/05/16 at 22:35:29, Randy Smith (Not in Mondays) wrote: > The fact that the implementation of this involves setting a state and calling into DoLoop() suggests to me that three are specific restrictions on when this can be called (e.g. not when another read is outstanding?). Could you document those conditions, or respond to this comment telling me why it's ok to just do a state transition by fiat at any point in the state machine to a new state? Documented and also added a DCHECK that state should be NONE. > > (I'd also value understanding better what the use cases are for uses of this function. That may not be appropriate in the documentation comment--the documentation comment should indicate what the effect of the function is from the consumers POV along with any restrictions the consumer needs to obey in calling it--but if not, respond to this comment with a sketch?) Since the active transaction can be destroyed at any time and if that was the only transaction associated with Writers then the entry should be correctly marked as truncated after the ongoing read/write is complete. I haven't yet added a call to it because based on integration with HttpCache, it could be invoked from HttpCache layer or from the Writers layer itself. That's a TODO. https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:97: // For testing. On 2017/05/16 at 22:35:29, Randy Smith (Not in Mondays) wrote: > nit: Test functions usually have "ForTesting" suffixed; I think there's some auto-analysis to confirm they're only used for testing. Added the suffix. https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:163: RequestPriority getCurrentHighestPriority(); On 2017/05/16 at 22:35:28, Randy Smith (Not in Mondays) wrote: > nit: I don't think this format is allowed by the style guide; it's either lower case separated by underscores (and usually implemented inline) or starting with a capital. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Next step of comments, this primarily being about the implementation. I may need to talk with you after the next round in being used; I have an internal sense from reading the code, but that sense may be inaccurate. https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:93: void TruncateEntry(); On 2017/05/17 13:02:30, shivanisha wrote: > On 2017/05/16 at 22:35:29, Randy Smith (Not in Mondays) wrote: > > The fact that the implementation of this involves setting a state and calling > into DoLoop() suggests to me that three are specific restrictions on when this > can be called (e.g. not when another read is outstanding?). Could you document > those conditions, or respond to this comment telling me why it's ok to just do a > state transition by fiat at any point in the state machine to a new state? > > Documented and also added a DCHECK that state should be NONE. > > > > (I'd also value understanding better what the use cases are for uses of this > function. That may not be appropriate in the documentation comment--the > documentation comment should indicate what the effect of the function is from > the consumers POV along with any restrictions the consumer needs to obey in > calling it--but if not, respond to this comment with a sketch?) > > Since the active transaction can be destroyed at any time and if that was the > only transaction associated with Writers then the entry should be correctly > marked as truncated after the ongoing read/write is complete. I haven't yet > added a call to it because based on integration with HttpCache, it could be > invoked from HttpCache layer or from the Writers layer itself. That's a TODO. > Can that be done as a conditional inside of RemoveTransaction() rather than as a separate interface? https://codereview.chromium.org/2886483002/diff/120001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/120001/net/http/http_cache_wr... net/http/http_cache_writers.h:92: // not currently in progress. Is it possible for a consumer to know this? Naively, I'd expect consumers would only know about their interactions with the SharedWriters class. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:28: DCHECK(!callback.is_null()); nit, suggestion: DCHECK(transaction) https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:39: DCHECK_EQ(next_state_, State::NONE); This (and the test above) seem wrong to me--if an active transaction can remove itself while a network transaction is outstanding, the test above should be for if a network transaction is outstanding/the state machine is active. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:40: DCHECK(callback_.is_null()); This feels redundant with the below same DCHECK; do we gain much from having it in both places? https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:43: read_buf_ = std::move(buf); Why std::move on a scoped_refptr<>? This is just taking another reference, right? It's not taking the buffer away from the caller. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:74: auto return_val = all_writers_.insert(transaction); nit, suggestion: I don't find the type of return_val intuitive here (i.e. I don't have the stl interface memorized), so I'd vote in favor of spelling it out. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:78: network_transaction_ = std::move(network_transaction); What drove adding the network transaction here rather than on construction? It's only the first HC::T that'll add a network transaction, so having it as part of the AddTransaction method feels a bit funny. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:97: callback_.Reset(); Is this really all that needs to happen if we're nuking the active transaction? I'm specifically concerned about the case where after a RemoveTransaction() a new transaction comes along and adds itself and tries to start driving the (busy) network transaction. Won't that cause a DCHECK() in read? https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:114: all_writers_.clear(); No need to nuke waiting_for_read_ or do anything with callbacks? https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:118: RequestPriority current_highest = GetCurrentHighestPriority(); nit, suggestion: My personal bias is that, if there's only one call site for a function and that function is fairly short, it should be hoisted into the call site location, possibly with a comment describing what it does (in place of the descriptive ability of the function name). https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:128: // writers. This reads to me as a requirement on the caller, and hence belonging in the declaration not the definition? https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:132: entry_->readers.insert(idle_writer); Hmm. I think this is dependent on the cache entry having been fully written, otherwise the readers are going to call back into Writers::Read(), aren't they? Should we assert that somehow, or more closely link moving idle writers to readers to when we finish filling in the cache entry? https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:139: DCHECK(!network_read_only_); Why? https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:154: SetIdleWritersFailState(error); Why have this logic in the consumers (HC::Ts) instead of in this class? The interface would be simpler if this class just got into a state where calls to Read() would just return |error|. That means that HC::Ts might read from the cache for a while before hitting the error, but I think that's a good thing? They might only end up needing what was in the cache. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:225: base::ResetAndReturn(&callback_).Run(rv); nit, suggestion: The pattern I've seen more commonly is that the DoLoop() has no responsibility for handling the callback; it's callers do it. Here, that responsibility is shared (between DoLoop() and Read(), at least) which opens a bit of confusion into which a bug could squeeze. So I'd suggest having DoLoop() just unilaterally return the rv, and have the callers do the right thing in terms of handling it (setting the callback on ERR_IO_PENDING in Read(), nulling and calling it on !ERR_IO_PENDING in the io callback). But up to you. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:247: // If no consumer, invoke entry processing here itself. suggestion, possibly not for this CL: I'd rather have this done from a single place rather than two; how reasonable is it to always do this call from here rather than having the active_transaction_ do it if it's still around? (Obviously, this applies to all DoneWithEntry/DoneWritingToEntry calls.) https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:249: cache_->DoneWithEntry(entry_, nullptr, true); Is there some path from here to ProcessFailure? How do the WaitingForRead transactions get notified? https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:271: // Transaction must be alive if this is a partial request. This isn't backed up with a DCHECK, and I'm not sure how we can guarantee that it's always true--is there any reason a partial transaction might not be destroyed while the network request is outstanding? How do we deal with that case? https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:333: ProcessWaitingForReadTransactions(write_len_); nit, suggestion: The grouping would feel slightly better to me if the copying and the notification were in the same function; it's all part of processing the waiting for read transactions. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:336: active_transaction_ = nullptr; Why keep the active transaction set if the end of the response has been hit? It would seem that once the network request returns, anything special about the active_transaction_ is finished, and it should be removed. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:356: bool HttpCache::Writers::IsResponseCompleted() { I'm a bit uncomfortable with this function's name; it's not testing whether the response is completed, it's testing whether it could be completed in the context of a 0 length read signaling EOF from the network transaction. This is a case where just hoisting the function up to its single call site would solve the problem, as getting rid of the confusion caused by the name is my goal :-}. Alternatively, would you call it something like "IsResponsePossiblyCompleted()" or something like that, and maybe put a comment here or at its declaration site indicating that it's testing for completion in the presence of an EOF signal. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:367: int HttpCache::Writers::WriteResponseInfo(bool truncated) { Again, I'd vote in favor of hoisting this into the calling function. There's nothing in that function, so I don't think there's any value in terms of readability or comprehensibility to leaving it here. If you disagree, I'll go with your preference, but if you're willing give me a sense of why it's your preference. If we have different aesthetics in this space I can probably review your code better if I understand your aesthetics. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:372: response->Persist(data->pickle(), skip_transient_headers, truncated); nit, suggestion: I'd just write this as response->Persist(data->pickle(), true /* skip_transient_headers *, truncated); Possibly with the comment immediately above that line (though I don't think the comment says much the code doesn't). https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:384: result = it->write_len; (Just an addition to an earlier comment: Setting result from write_len seems like it is pretty directly related to how many bytes were copied. If you did the copying and posting in one function, you wouldn't need the temporary data member.) https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:388: all_writers_.erase(transaction); Caption: This comment is an extended babble with me trying to get my thoughts in order and possibly spark thoughts in you--if you'd like to ignore it for now, that's fine, and I'll come back to this issue when I understand this patch better and how it's going to be used. I'm a bit uncomfortable with the responsibility for keeping all_writers_ up to date being shared between consumer and producer. Given that the consumer has to remove itself upon destruction, is it reasonable to expect it to remove itself upon error as well? Or leave itself on the list until it's destroyed (which probably won't be very long)? Or does that cause problems in other areas? This fits in with a bit of discomfort about transactions both being listed in the Writers and being passed into Read(), which makes me want to explore whether or not we really need to have all_writers_. It looks to me as if that's only used for StopCaching() and MoveIdleWritersToReaders(). StopCaching() is a bit weird in that the first transaction in may call it, and thus forbid later transactions, but later transactions don't have that option, so there's an asymmetry. And MoveIdleWritersToReaders() seems really weird to me, since HC::Ts can move back and forth between being readers and writers based on how far ahead of their read point the cache entry gets. So I find myself wanting to explore whether there's an architecture that uses Writers as a synchronization point, but doesn't keep a list of writers at all; it only keeps the active transaction and the list of WaitingToRead transactions. I guess I'll ask: How do you envision the additions and removals to this class (and to the queues in ActiveEntry) occurring if: * Transaction A does validation, reads a chunk of data from the network. * Transaction B does validation, reads a lot of chunks of data from the network, not completely filling the entry. * Transaction A comes back and reads data, now from the cache. Transaction A will stay in all_writers_, but will be reading from the cache. What do we gain from having it in all_writers_? https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.h:81: void SetExclusive() { is_exclusive_ = true; } What happens if this is called with a lot of transactions on the object? It would seem like more handling than just setting the bit would be necessary; can you give me a sense as to why that isn't true? https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.h:183: ActiveEntry* entry_ = nullptr; My understanding is that Chromium prefers to keep initialization together (i.e. either all in the .h file or all in the constructor). Having said that, there's a broken pointer link (apparently the Google C++ guide used to comment on this and now it doesn't) so if you feel strongly about keeping them separate ping me about the issue and I'll ping the thread (or ping the thread directly: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0). https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.h:215: CompletionCallback callback_; // Consumer's callback. Suggestion: Change comment to "Callback for active_transaction_". https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.h:216: CompletionCallback io_callback_; Suggestion: In my ideal world this would be const, since it's initialized in the constructor and never touched after that. That might mean you need to initialize it in the constructor data member list, which might mean you need to put it after WeakPtrFactory, or maybe cast? But I think marking this const would be a useful signal to future readers. An alternative would be to actually just replicate the Bind() call in all three places where this is used, which I'd be completely fine with but I realize is inconsistent with usage elsewhere in the cache. More code, but IMO more readable code (since the reader doesn't have to lookup the io_callback_ initialization).
Patchset #7 (id:180001) has been deleted
PTAL, Thanks! Addressed some of the feedback. Will be responding to the remaining in the next patch. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:28: DCHECK(!callback.is_null()); On 2017/05/18 at 01:02:55, Randy Smith (Not in Mondays) wrote: > nit, suggestion: DCHECK(transaction) done https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:39: DCHECK_EQ(next_state_, State::NONE); On 2017/05/18 at 01:02:55, Randy Smith (Not in Mondays) wrote: > This (and the test above) seem wrong to me--if an active transaction can remove itself while a network transaction is outstanding, the test above should be for if a network transaction is outstanding/the state machine is active. Ah, that's right. Changed the condition to next_state_ != State::NONE https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:40: DCHECK(callback_.is_null()); On 2017/05/18 at 01:02:55, Randy Smith (Not in Mondays) wrote: > This feels redundant with the below same DCHECK; do we gain much from having it in both places? removed from below https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:43: read_buf_ = std::move(buf); On 2017/05/18 at 01:02:55, Randy Smith (Not in Mondays) wrote: > Why std::move on a scoped_refptr<>? This is just taking another reference, right? It's not taking the buffer away from the caller. Its just taking the buffer away from the argument which would have anyways happened when the function ended, (not from the caller). That's how I understood it would work. Let me know if its wrong. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:74: auto return_val = all_writers_.insert(transaction); On 2017/05/18 at 01:02:55, Randy Smith (Not in Mondays) wrote: > nit, suggestion: I don't find the type of return_val intuitive here (i.e. I don't have the stl interface memorized), so I'd vote in favor of spelling it out. done https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:78: network_transaction_ = std::move(network_transaction); On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > What drove adding the network transaction here rather than on construction? It's only the first HC::T that'll add a network transaction, so having it as part of the AddTransaction method feels a bit funny. I am hoping writers can be a member variable of ActiveEntry instead of a unique_ptr and in that case writers will be created even before a transaction is added to it. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:97: callback_.Reset(); On 2017/05/18 at 01:02:55, Randy Smith (Not in Mondays) wrote: > Is this really all that needs to happen if we're nuking the active transaction? I'm specifically concerned about the case where after a RemoveTransaction() a new transaction comes along and adds itself and tries to start driving the (busy) network transaction. Won't that cause a DCHECK() in read? Updated Read to see that if next_state_ is not NONE , consider the network transaction to be busy. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:114: all_writers_.clear(); On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > No need to nuke waiting_for_read_ or do anything with callbacks? On second thoughts, I actually do not see a need for this API. Removing it unless there is a need to do this. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:118: RequestPriority current_highest = GetCurrentHighestPriority(); On 2017/05/18 at 01:02:55, Randy Smith (Not in Mondays) wrote: > nit, suggestion: My personal bias is that, if there's only one call site for a function and that function is fairly short, it should be hoisted into the call site location, possibly with a comment describing what it does (in place of the descriptive ability of the function name). sgtm, done https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:128: // writers. On 2017/05/18 at 01:02:55, Randy Smith (Not in Mondays) wrote: > This reads to me as a requirement on the caller, and hence belonging in the declaration not the definition? done in the comment and also added a function ContainsOnlyIdleWriters() to be used by the caller. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:132: entry_->readers.insert(idle_writer); On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > Hmm. I think this is dependent on the cache entry having been fully written, otherwise the readers are going to call back into Writers::Read(), aren't they? Should we assert that somehow, or more closely link moving idle writers to readers to when we finish filling in the cache entry? In CL https://codereview.chromium.org/2721933002/ when HC::T invokes DoneWritingToEntry(), HttpCache takes care of checking that it is indeed a headers_transaction or a writer , nulls the corresponding pointer field and then works on other queued transactions. I am envisioning the integration of the parallel validation CL and this CL s.t. HC::T will still be responsible for invoking DoneWritingToEntry(), HttpCache then takes care of checking that it is indeed a headers_transaction or one of the writers , nulls the corresponding pointer field or calls RemoveTransaction() on writers and then invokes works on other queued transactions. As part of working on other queued transactions, it will also invoke MoveIdleWritersToReaders(). So the flow of events still stay the same, except there is a set of writers to work on instead of a single writer and in addition to adding just the done_headers_queue transactions to readers, it also adds the idle writers. Let me know how this sounds. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:139: DCHECK(!network_read_only_); On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > Why? I believe this should be changed to network_read_only_ = false, to take care of a race where network_read_only_ was set to true , then all transactions left and then before entry could be destroyed a new transaction needs to be added, it should be allowed. Not completely sure of this decision though. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:154: SetIdleWritersFailState(error); On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > Why have this logic in the consumers (HC::Ts) instead of in this class? The interface would be simpler if this class just got into a state where calls to Read() would just return |error|. That means that HC::Ts might read from the cache for a while before hitting the error, but I think that's a good thing? They might only end up needing what was in the cache. Its possible that entry is destroyed and then only HC::T is left to take care of a future read. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:225: base::ResetAndReturn(&callback_).Run(rv); On 2017/05/18 at 01:02:55, Randy Smith (Not in Mondays) wrote: > nit, suggestion: The pattern I've seen more commonly is that the DoLoop() has no responsibility for handling the callback; it's callers do it. Here, that responsibility is shared (between DoLoop() and Read(), at least) which opens a bit of confusion into which a bug could squeeze. So I'd suggest having DoLoop() just unilaterally return the rv, and have the callers do the right thing in terms of handling it (setting the callback on ERR_IO_PENDING in Read(), nulling and calling it on !ERR_IO_PENDING in the io callback). But up to you. I guess I am only much aware of the way it is handled in HC::T and using the same pattern here. I am not completely certain if I understand your suggestion. I like this pattern though because invoking the consumer's callback is in a single place. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:247: // If no consumer, invoke entry processing here itself. On 2017/05/18 at 01:02:55, Randy Smith (Not in Mondays) wrote: > suggestion, possibly not for this CL: I'd rather have this done from a single place rather than two; how reasonable is it to always do this call from here rather than having the active_transaction_ do it if it's still around? > > (Obviously, this applies to all DoneWithEntry/DoneWritingToEntry calls.) I will look at this suggestion again in the integration CL. Adding a TODO here. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:249: cache_->DoneWithEntry(entry_, nullptr, true); On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > Is there some path from here to ProcessFailure? How do the WaitingForRead transactions get notified? DoneWithEntry => DoneWritingToEntry => ProcessFailure along with invoking ProcessEntryFailure (in CL: https://codereview.chromium.org/2721933002/) https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:271: // Transaction must be alive if this is a partial request. On 2017/05/18 at 01:02:55, Randy Smith (Not in Mondays) wrote: > This isn't backed up with a DCHECK, and I'm not sure how we can guarantee that it's always true--is there any reason a partial transaction might not be destroyed while the network request is outstanding? How do we deal with that case? In case of partial requests, SetExclusive will be invoked and thus when the single transaction is removed, no other transaction will invoke Read on this. Also resetting the network transaction when the transaction is removed so that if there is an ongoing read, it should be destroyed.
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, Thanks! Latest patch has all of the feedback addressed. https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/100001/net/http/http_cache_wr... net/http/http_cache_writers.h:93: void TruncateEntry(); On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > On 2017/05/17 13:02:30, shivanisha wrote: > > On 2017/05/16 at 22:35:29, Randy Smith (Not in Mondays) wrote: > > > The fact that the implementation of this involves setting a state and calling > > into DoLoop() suggests to me that three are specific restrictions on when this > > can be called (e.g. not when another read is outstanding?). Could you document > > those conditions, or respond to this comment telling me why it's ok to just do a > > state transition by fiat at any point in the state machine to a new state? > > > > Documented and also added a DCHECK that state should be NONE. > > > > > > (I'd also value understanding better what the use cases are for uses of this > > function. That may not be appropriate in the documentation comment--the > > documentation comment should indicate what the effect of the function is from > > the consumers POV along with any restrictions the consumer needs to obey in > > calling it--but if not, respond to this comment with a sketch?) > > > > Since the active transaction can be destroyed at any time and if that was the > > only transaction associated with Writers then the entry should be correctly > > marked as truncated after the ongoing read/write is complete. I haven't yet > > added a call to it because based on integration with HttpCache, it could be > > invoked from HttpCache layer or from the Writers layer itself. That's a TODO. > > > > Can that be done as a conditional inside of RemoveTransaction() rather than as a separate interface? RemoveTransaction() will be called as soon as the transaction is removed while TruncateEntry should be invoked after the ongoing cache write is complete. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:154: SetIdleWritersFailState(error); On 2017/05/18 at 20:59:44, shivanisha wrote: > On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > > Why have this logic in the consumers (HC::Ts) instead of in this class? The interface would be simpler if this class just got into a state where calls to Read() would just return |error|. That means that HC::Ts might read from the cache for a while before hitting the error, but I think that's a good thing? They might only end up needing what was in the cache. > > Its possible that entry is destroyed and then only HC::T is left to take care of a future read. Or we can doom this entry but let these transaction still associated so that Active Entry is not destroyed? https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:247: // If no consumer, invoke entry processing here itself. On 2017/05/18 at 20:59:43, shivanisha wrote: > On 2017/05/18 at 01:02:55, Randy Smith (Not in Mondays) wrote: > > suggestion, possibly not for this CL: I'd rather have this done from a single place rather than two; how reasonable is it to always do this call from here rather than having the active_transaction_ do it if it's still around? > > > > (Obviously, this applies to all DoneWithEntry/DoneWritingToEntry calls.) > > I will look at this suggestion again in the integration CL. Adding a TODO here. Actually on second thoughts, its simpler to invoke it from here. Changing all sites to invoke DoneWithEntry/DoneWritingToEntry unconditionally from writers. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:333: ProcessWaitingForReadTransactions(write_len_); On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > nit, suggestion: The grouping would feel slightly better to me if the copying and the notification were in the same function; it's all part of processing the waiting for read transactions. done https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:336: active_transaction_ = nullptr; On 2017/05/18 at 01:02:55, Randy Smith (Not in Mondays) wrote: > Why keep the active transaction set if the end of the response has been hit? It would seem that once the network request returns, anything special about the active_transaction_ is finished, and it should be removed. Now that DoneWithEntry/DoneWritingToEntry will be invoked from here , no need to retain this pointer. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:356: bool HttpCache::Writers::IsResponseCompleted() { On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > I'm a bit uncomfortable with this function's name; it's not testing whether the response is completed, it's testing whether it could be completed in the context of a 0 length read signaling EOF from the network transaction. > > This is a case where just hoisting the function up to its single call site would solve the problem, as getting rid of the confusion caused by the name is my goal :-}. Alternatively, would you call it something like "IsResponsePossiblyCompleted()" or something like that, and maybe put a comment here or at its declaration site indicating that it's testing for completion in the presence of an EOF signal. Moved it to its call site. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:367: int HttpCache::Writers::WriteResponseInfo(bool truncated) { On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > Again, I'd vote in favor of hoisting this into the calling function. There's nothing in that function, so I don't think there's any value in terms of readability or comprehensibility to leaving it here. If you disagree, I'll go with your preference, but if you're willing give me a sense of why it's your preference. If we have different aesthetics in this space I can probably review your code better if I understand your aesthetics. Moved to calling site. I had this as separate function mainly because I thought it would be needed by both HC::T and this class but I think its simple enough to be in both classes. I had thought the same for IsResponseCompleted but looks like that would not be needed by HC::T. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:372: response->Persist(data->pickle(), skip_transient_headers, truncated); On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > nit, suggestion: I'd just write this as > > response->Persist(data->pickle(), true /* skip_transient_headers *, truncated); > > Possibly with the comment immediately above that line (though I don't think the comment says much the code doesn't). done https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:384: result = it->write_len; On 2017/05/18 at 01:02:55, Randy Smith (Not in Mondays) wrote: > (Just an addition to an earlier comment: Setting result from write_len seems like it is pretty directly related to how many bytes were copied. If you did the copying and posting in one function, you wouldn't need the temporary data member.) Moved copying to this function. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:388: all_writers_.erase(transaction); On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > Caption: This comment is an extended babble with me trying to get my thoughts in order and possibly spark thoughts in you--if you'd like to ignore it for now, that's fine, and I'll come back to this issue when I understand this patch better and how it's going to be used. > > I'm a bit uncomfortable with the responsibility for keeping all_writers_ up to date being shared between consumer and producer. Given that the consumer has to remove itself upon destruction, is it reasonable to expect it to remove itself upon error as well? Or leave itself on the list until it's destroyed (which probably won't be very long)? Or does that cause problems in other areas? > > This fits in with a bit of discomfort about transactions both being listed in the Writers and being passed into Read(), which makes me want to explore whether or not we really need to have all_writers_. It looks to me as if that's only used for StopCaching() and MoveIdleWritersToReaders(). StopCaching() is a bit weird in that the first transaction in may call it, and thus forbid later transactions, but later transactions don't have that option, so there's an asymmetry. And MoveIdleWritersToReaders() seems really weird to me, since HC::Ts can move back and forth between being readers and writers based on how far ahead of their read point the cache entry gets. So I find myself wanting to explore whether there's an architecture that uses Writers as a synchronization point, but doesn't keep a list of writers at all; it only keeps the active transaction and the list of WaitingToRead transactions. > > I guess I'll ask: How do you envision the additions and removals to this class (and to the queues in ActiveEntry) occurring if: > > * Transaction A does validation, reads a chunk of data from the network. > * Transaction B does validation, reads a lot of chunks of data from the network, not completely filling the entry. > * Transaction A comes back and reads data, now from the cache. > > Transaction A will stay in all_writers_, but will be reading from the cache. What do we gain from having it in all_writers_? In case transaction B comes back , reads from network and fails writing to cache, writers will need to know the transactions that should be notified of this failure.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:388: all_writers_.erase(transaction); On 2017/05/19 at 13:49:58, shivanisha wrote: > On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > > Caption: This comment is an extended babble with me trying to get my thoughts in order and possibly spark thoughts in you--if you'd like to ignore it for now, that's fine, and I'll come back to this issue when I understand this patch better and how it's going to be used. > > > > I'm a bit uncomfortable with the responsibility for keeping all_writers_ up to date being shared between consumer and producer. Given that the consumer has to remove itself upon destruction, is it reasonable to expect it to remove itself upon error as well? Or leave itself on the list until it's destroyed (which probably won't be very long)? Or does that cause problems in other areas? > > > > This fits in with a bit of discomfort about transactions both being listed in the Writers and being passed into Read(), which makes me want to explore whether or not we really need to have all_writers_. It looks to me as if that's only used for StopCaching() and MoveIdleWritersToReaders(). StopCaching() is a bit weird in that the first transaction in may call it, and thus forbid later transactions, but later transactions don't have that option, so there's an asymmetry. And MoveIdleWritersToReaders() seems really weird to me, since HC::Ts can move back and forth between being readers and writers based on how far ahead of their read point the cache entry gets. So I find myself wanting to explore whether there's an architecture that uses Writers as a synchronization point, but doesn't keep a list of writers at all; it only keeps the active transaction and the list of WaitingToRead transactions. > > > > I guess I'll ask: How do you envision the additions and removals to this class (and to the queues in ActiveEntry) occurring if: > > > > * Transaction A does validation, reads a chunk of data from the network. > > * Transaction B does validation, reads a lot of chunks of data from the network, not completely filling the entry. > > * Transaction A comes back and reads data, now from the cache. > > > > Transaction A will stay in all_writers_, but will be reading from the cache. What do we gain from having it in all_writers_? > > In case transaction B comes back , reads from network and fails writing to cache, writers will need to know the transactions that should be notified of this failure. Actually, keeping all_writers_ is important from a high level perspective for the following invariant to be true: Currently, entry->writer != nullptr means there is a writer. After shared writing, !entry->writers.IsEmpty() should signify that there is a writer present. If we do not save this then while adding a new transaction it won't know if a writer is present.
Sorry, I had intended to get this code review done before the end of the weekend, and I'm afraid that's not going to happen. Here's the in-progress review--mostly it's a ping on a couple of comments I didn't see responses to. I'll try to finish this review early Tuesday. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:39: DCHECK_EQ(next_state_, State::NONE); On 2017/05/18 20:59:44, shivanisha wrote: > On 2017/05/18 at 01:02:55, Randy Smith (Not in Mondays) wrote: > > This (and the test above) seem wrong to me--if an active transaction can > remove itself while a network transaction is outstanding, the test above should > be for if a network transaction is outstanding/the state machine is active. > > Ah, that's right. Changed the condition to next_state_ != State::NONE I'm not seeing the change (looking at PS6->8 diff)? https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:132: entry_->readers.insert(idle_writer); On 2017/05/18 20:59:44, shivanisha wrote: > On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > > Hmm. I think this is dependent on the cache entry having been fully written, > otherwise the readers are going to call back into Writers::Read(), aren't they? > Should we assert that somehow, or more closely link moving idle writers to > readers to when we finish filling in the cache entry? > > In CL https://codereview.chromium.org/2721933002/ when HC::T invokes > DoneWritingToEntry(), HttpCache takes care of checking that it is indeed a > headers_transaction or a writer , nulls the corresponding pointer field and then > works on other queued transactions. > > I am envisioning the integration of the parallel validation CL and this CL s.t. > HC::T will still be responsible for invoking DoneWritingToEntry(), HttpCache > then takes care of checking that it is indeed a headers_transaction or one of > the writers , nulls the corresponding pointer field or calls RemoveTransaction() > on writers and then invokes works on other queued transactions. As part of > working on other queued transactions, it will also invoke > MoveIdleWritersToReaders(). > > So the flow of events still stay the same, except there is a set of writers to > work on instead of a single writer and in addition to adding just the > done_headers_queue transactions to readers, it also adds the idle writers. Let > me know how this sounds. SGTM. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:139: DCHECK(!network_read_only_); On 2017/05/18 20:59:43, shivanisha wrote: > On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > > Why? > > I believe this should be changed to network_read_only_ = false, to take care of > a race where network_read_only_ was set to true , then all transactions left and > then before entry could be destroyed a new transaction needs to be added, it > should be allowed. Not completely sure of this decision though. I'd be more inclined to say that network_read_only_ should be set to false whenever all_writers_ transitions to empty; at that point the transaction that set it to true is no longer relevant. (Which, amusingly, answers my "Why?" question above--if there aren't any writers, that implies we should have reset it. Though it sounds like that's not what you were thinking.) https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:388: all_writers_.erase(transaction); On 2017/05/19 17:12:58, shivanisha wrote: > On 2017/05/19 at 13:49:58, shivanisha wrote: > > On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > > > Caption: This comment is an extended babble with me trying to get my > thoughts in order and possibly spark thoughts in you--if you'd like to ignore it > for now, that's fine, and I'll come back to this issue when I understand this > patch better and how it's going to be used. > > > > > > I'm a bit uncomfortable with the responsibility for keeping all_writers_ up > to date being shared between consumer and producer. Given that the consumer has > to remove itself upon destruction, is it reasonable to expect it to remove > itself upon error as well? Or leave itself on the list until it's destroyed > (which probably won't be very long)? Or does that cause problems in other > areas? > > > > > > This fits in with a bit of discomfort about transactions both being listed > in the Writers and being passed into Read(), which makes me want to explore > whether or not we really need to have all_writers_. It looks to me as if that's > only used for StopCaching() and MoveIdleWritersToReaders(). StopCaching() is a > bit weird in that the first transaction in may call it, and thus forbid later > transactions, but later transactions don't have that option, so there's an > asymmetry. And MoveIdleWritersToReaders() seems really weird to me, since > HC::Ts can move back and forth between being readers and writers based on how > far ahead of their read point the cache entry gets. So I find myself wanting to > explore whether there's an architecture that uses Writers as a synchronization > point, but doesn't keep a list of writers at all; it only keeps the active > transaction and the list of WaitingToRead transactions. > > > > > > I guess I'll ask: How do you envision the additions and removals to this > class (and to the queues in ActiveEntry) occurring if: > > > > > > * Transaction A does validation, reads a chunk of data from the network. > > > * Transaction B does validation, reads a lot of chunks of data from the > network, not completely filling the entry. > > > * Transaction A comes back and reads data, now from the cache. > > > > > > Transaction A will stay in all_writers_, but will be reading from the cache. > What do we gain from having it in all_writers_? > > > > In case transaction B comes back , reads from network and fails writing to > cache, writers will need to know the transactions that should be notified of > this failure. > > Actually, keeping all_writers_ is important from a high level perspective for > the following invariant to be true: > Currently, entry->writer != nullptr means there is a writer. > After shared writing, !entry->writers.IsEmpty() should signify that there is a > writer present. If we do not save this then while adding a new transaction it > won't know if a writer is present. > Acknowledged. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.h:81: void SetExclusive() { is_exclusive_ = true; } On 2017/05/18 01:02:56, Randy Smith (Not in Mondays) wrote: > What happens if this is called with a lot of transactions on the object? It > would seem like more handling than just setting the bit would be necessary; can > you give me a sense as to why that isn't true? Ping? https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.h:183: ActiveEntry* entry_ = nullptr; On 2017/05/18 01:02:55, Randy Smith (Not in Mondays) wrote: > My understanding is that Chromium prefers to keep initialization together (i.e. > either all in the .h file or all in the constructor). Having said that, there's > a broken pointer link (apparently the Google C++ guide used to comment on this > and now it doesn't) so if you feel strongly about keeping them separate ping me > about the issue and I'll ping the thread (or ping the thread directly: > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0). > Ping? https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.h:215: CompletionCallback callback_; // Consumer's callback. On 2017/05/18 01:02:55, Randy Smith (Not in Mondays) wrote: > Suggestion: Change comment to "Callback for active_transaction_". Ping? https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.h:216: CompletionCallback io_callback_; On 2017/05/18 01:02:56, Randy Smith (Not in Mondays) wrote: > Suggestion: In my ideal world this would be const, since it's initialized in the > constructor and never touched after that. That might mean you need to > initialize it in the constructor data member list, which might mean you need to > put it after WeakPtrFactory, or maybe cast? But I think marking this const > would be a useful signal to future readers. > > An alternative would be to actually just replicate the Bind() call in all three > places where this is used, which I'd be completely fine with but I realize is > inconsistent with usage elsewhere in the cache. More code, but IMO more > readable code (since the reader doesn't have to lookup the io_callback_ > initialization). Ping?
Shivani: Full followup on existing comments. Let's run through these, then I'll do another from scratch review. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:43: read_buf_ = std::move(buf); On 2017/05/18 20:59:44, shivanisha wrote: > On 2017/05/18 at 01:02:55, Randy Smith (Not in Mondays) wrote: > > Why std::move on a scoped_refptr<>? This is just taking another reference, > right? It's not taking the buffer away from the caller. > > Its just taking the buffer away from the argument which would have anyways > happened when the function ended, (not from the caller). That's how I understood > it would work. Let me know if its wrong. Acknowledged. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:78: network_transaction_ = std::move(network_transaction); On 2017/05/18 20:59:44, shivanisha wrote: > On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > > What drove adding the network transaction here rather than on construction? > It's only the first HC::T that'll add a network transaction, so having it as > part of the AddTransaction method feels a bit funny. > > I am hoping writers can be a member variable of ActiveEntry instead of a > unique_ptr and in that case writers will be created even before a transaction is > added to it. Hmmm. Ok. There's a range of design choices here, and I don't have a strong feeling that one is better than the other, but keep the other ones (unique_ptr, separate methods for adding network transaction & HC::Ts, separate methods for adding the first transaction and adding subsequent ones, this method but destroying other network transactions when they come in) in mind as possibilities and make a conscious decision among them. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:78: network_transaction_ = std::move(network_transaction); Worthwhile adding a DCHECK that network_transaction_ wasn't already set? https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:154: SetIdleWritersFailState(error); On 2017/05/19 13:49:57, shivanisha wrote: > On 2017/05/18 at 20:59:44, shivanisha wrote: > > On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > > > Why have this logic in the consumers (HC::Ts) instead of in this class? The > interface would be simpler if this class just got into a state where calls to > Read() would just return |error|. That means that HC::Ts might read from the > cache for a while before hitting the error, but I think that's a good thing? > They might only end up needing what was in the cache. > > > > Its possible that entry is destroyed and then only HC::T is left to take care > of a future read. > > Or we can doom this entry but let these transaction still associated so that > Active Entry is not destroyed? Yeah, that was the direction I think I was gesturing in. But I don't think it's worth a lot of complex coding/changing current implementation a lot. Could you sketch out to me the path which would resulting in destroying the entry while there are HC::Ts that were once associated with it? https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:225: base::ResetAndReturn(&callback_).Run(rv); On 2017/05/18 20:59:44, shivanisha wrote: > On 2017/05/18 at 01:02:55, Randy Smith (Not in Mondays) wrote: > > nit, suggestion: The pattern I've seen more commonly is that the DoLoop() has > no responsibility for handling the callback; it's callers do it. Here, that > responsibility is shared (between DoLoop() and Read(), at least) which opens a > bit of confusion into which a bug could squeeze. So I'd suggest having DoLoop() > just unilaterally return the rv, and have the callers do the right thing in > terms of handling it (setting the callback on ERR_IO_PENDING in Read(), nulling > and calling it on !ERR_IO_PENDING in the io callback). But up to you. > > I guess I am only much aware of the way it is handled in HC::T and using the > same pattern here. I am not completely certain if I understand your suggestion. > I like this pattern though because invoking the consumer's callback is in a > single place. Acknowledged. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:249: cache_->DoneWithEntry(entry_, nullptr, true); On 2017/05/18 20:59:44, shivanisha wrote: > On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > > Is there some path from here to ProcessFailure? How do the WaitingForRead > transactions get notified? > > DoneWithEntry => DoneWritingToEntry => ProcessFailure along with invoking > ProcessEntryFailure (in CL: https://codereview.chromium.org/2721933002/) Acknowledged. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:271: // Transaction must be alive if this is a partial request. On 2017/05/18 20:59:43, shivanisha wrote: > On 2017/05/18 at 01:02:55, Randy Smith (Not in Mondays) wrote: > > This isn't backed up with a DCHECK, and I'm not sure how we can guarantee that > it's always true--is there any reason a partial transaction might not be > destroyed while the network request is outstanding? How do we deal with that > case? > > In case of partial requests, SetExclusive will be invoked and thus when the > single transaction is removed, no other transaction will invoke Read on this. > Also resetting the network transaction when the transaction is removed so that > if there is an ongoing read, it should be destroyed. Thanks for the explanation. (nit, suggestion:) I think the above comment would be improved by a few words from that explanation, maybe just saying "The active transaction must be alive if this is a partial request, as partial requests are exclusive and hence will always be the active transaction." (with the todo?) . But up to you if you think that's worth it. https://codereview.chromium.org/2886483002/diff/220001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/220001/net/http/http_cache_wr... net/http/http_cache_writers.cc:99: network_transaction_.reset(); If this is done without changing state_ can't that leave the state machine waiting for a network transaction callback that never happens? https://codereview.chromium.org/2886483002/diff/220001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/220001/net/http/http_cache_wr... net/http/http_cache_writers.h:75: // When response is completely written, any idle writers are moved to nit, suggestion: Rephrase to "Move any idle writers to entry_->readers. Should only be invoked when a response is completely written, i.e. when ContainesOnlyIdleWriters() returns true." (If that's an accurate statement.)
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, Thanks! Latest patch has Randy's feedback addressed. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:39: DCHECK_EQ(next_state_, State::NONE); On 2017/05/22 at 01:56:55, Randy Smith (Not in Mondays) wrote: > On 2017/05/18 20:59:44, shivanisha wrote: > > On 2017/05/18 at 01:02:55, Randy Smith (Not in Mondays) wrote: > > > This (and the test above) seem wrong to me--if an active transaction can > > remove itself while a network transaction is outstanding, the test above should > > be for if a network transaction is outstanding/the state machine is active. > > > > Ah, that's right. Changed the condition to next_state_ != State::NONE > > I'm not seeing the change (looking at PS6->8 diff)? It is present in PS8. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:78: network_transaction_ = std::move(network_transaction); On 2017/05/24 at 23:09:18, Randy Smith (Not in Mondays) wrote: > Worthwhile adding a DCHECK that network_transaction_ wasn't already set? Done https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:78: network_transaction_ = std::move(network_transaction); On 2017/05/24 at 23:09:18, Randy Smith (Not in Mondays) wrote: > Worthwhile adding a DCHECK that network_transaction_ wasn't already set? Added the dcheck https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:139: DCHECK(!network_read_only_); On 2017/05/22 at 01:56:55, Randy Smith (Not in Mondays) wrote: > On 2017/05/18 20:59:43, shivanisha wrote: > > On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > > > Why? > > > > I believe this should be changed to network_read_only_ = false, to take care of > > a race where network_read_only_ was set to true , then all transactions left and > > then before entry could be destroyed a new transaction needs to be added, it > > should be allowed. Not completely sure of this decision though. > > I'd be more inclined to say that network_read_only_ should be set to false whenever all_writers_ transitions to empty; at that point the transaction that set it to true is no longer relevant. (Which, amusingly, answers my "Why?" question above--if there aren't any writers, that implies we should have reset it. Though it sounds like that's not what you were thinking.) Added a function ResetStateForEmptyWriters which is called whenever all_writers_ is empty. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:154: SetIdleWritersFailState(error); On 2017/05/24 at 23:09:18, Randy Smith (Not in Mondays) wrote: > On 2017/05/19 13:49:57, shivanisha wrote: > > On 2017/05/18 at 20:59:44, shivanisha wrote: > > > On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > > > > Why have this logic in the consumers (HC::Ts) instead of in this class? The > > interface would be simpler if this class just got into a state where calls to > > Read() would just return |error|. That means that HC::Ts might read from the > > cache for a while before hitting the error, but I think that's a good thing? > > They might only end up needing what was in the cache. > > > > > > Its possible that entry is destroyed and then only HC::T is left to take care > > of a future read. > > > > Or we can doom this entry but let these transaction still associated so that > > Active Entry is not destroyed? > > Yeah, that was the direction I think I was gesturing in. But I don't think it's worth a lot of complex coding/changing current implementation a lot. Could you sketch out to me the path which would resulting in destroying the entry while there are HC::Ts that were once associated with it? If there is a failure then waiting_for_read_ transactions' callbacks are invoked with the error, idle transactions have a state set which will be checked in their future Read calls and Writers is then empty. Writers will then invoke cache_->DoneWritingToEntry with success as false leading to ProcessEntryFailure being called which will destroy the entry and restart any add_to_entry_queue transactions. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:271: // Transaction must be alive if this is a partial request. On 2017/05/24 at 23:09:18, Randy Smith (Not in Mondays) wrote: > On 2017/05/18 20:59:43, shivanisha wrote: > > On 2017/05/18 at 01:02:55, Randy Smith (Not in Mondays) wrote: > > > This isn't backed up with a DCHECK, and I'm not sure how we can guarantee that > > it's always true--is there any reason a partial transaction might not be > > destroyed while the network request is outstanding? How do we deal with that > > case? > > > > In case of partial requests, SetExclusive will be invoked and thus when the > > single transaction is removed, no other transaction will invoke Read on this. > > Also resetting the network transaction when the transaction is removed so that > > if there is an ongoing read, it should be destroyed. > > Thanks for the explanation. > (nit, suggestion:) I think the above comment would be improved by a few words from that explanation, maybe just saying "The active transaction must be alive if this is a partial request, as partial requests are exclusive and hence will always be the active transaction." (with the todo?) . But up to you if you think that's worth it. done https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.h:81: void SetExclusive() { is_exclusive_ = true; } On 2017/05/22 at 01:56:55, Randy Smith (Not in Mondays) wrote: > On 2017/05/18 01:02:56, Randy Smith (Not in Mondays) wrote: > > What happens if this is called with a lot of transactions on the object? It > > would seem like more handling than just setting the bit would be necessary; can > > you give me a sense as to why that isn't true? > > Ping? Added a dcheck that only 0 or 1 transactions should be present when this is called. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.h:183: ActiveEntry* entry_ = nullptr; On 2017/05/22 at 01:56:56, Randy Smith (Not in Mondays) wrote: > On 2017/05/18 01:02:55, Randy Smith (Not in Mondays) wrote: > > My understanding is that Chromium prefers to keep initialization together (i.e. > > either all in the .h file or all in the constructor). Having said that, there's > > a broken pointer link (apparently the Google C++ guide used to comment on this > > and now it doesn't) so if you feel strongly about keeping them separate ping me > > about the issue and I'll ping the thread (or ping the thread directly: > > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0). > > > > Ping? Changed the header file such that all fields are initialized there (except the ones that are initialized by default) https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.h:215: CompletionCallback callback_; // Consumer's callback. On 2017/05/22 at 01:56:56, Randy Smith (Not in Mondays) wrote: > On 2017/05/18 01:02:55, Randy Smith (Not in Mondays) wrote: > > Suggestion: Change comment to "Callback for active_transaction_". > > Ping? done https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.h:216: CompletionCallback io_callback_; On 2017/05/22 at 01:56:56, Randy Smith (Not in Mondays) wrote: > On 2017/05/18 01:02:56, Randy Smith (Not in Mondays) wrote: > > Suggestion: In my ideal world this would be const, since it's initialized in the > > constructor and never touched after that. That might mean you need to > > initialize it in the constructor data member list, which might mean you need to > > put it after WeakPtrFactory, or maybe cast? But I think marking this const > > would be a useful signal to future readers. > > > > An alternative would be to actually just replicate the Bind() call in all three > > places where this is used, which I'd be completely fine with but I realize is > > inconsistent with usage elsewhere in the cache. More code, but IMO more > > readable code (since the reader doesn't have to lookup the io_callback_ > > initialization). > > Ping? It seems nothing should come after WeakPtrFactory in the class as per https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?q=WeakPtrFactory&... Adding initialization on all 3 call sites. https://codereview.chromium.org/2886483002/diff/220001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/220001/net/http/http_cache_wr... net/http/http_cache_writers.cc:99: network_transaction_.reset(); On 2017/05/24 at 23:09:19, Randy Smith (Not in Mondays) wrote: > If this is done without changing state_ can't that leave the state machine waiting for a network transaction callback that never happens? Good catch. I can't seem to remember why I added it in the patch 7 but it shouldn't be here since even if active_transaction_ is destroyed, network transaction should continue to live unless all_writers_ is empty after this transaction is removed. Moving it to ResetStateForEmptyWriters() https://codereview.chromium.org/2886483002/diff/220001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/220001/net/http/http_cache_wr... net/http/http_cache_writers.h:75: // When response is completely written, any idle writers are moved to On 2017/05/24 at 23:09:19, Randy Smith (Not in Mondays) wrote: > nit, suggestion: Rephrase to "Move any idle writers to entry_->readers. Should only be invoked when a response is completely written, i.e. when ContainesOnlyIdleWriters() returns true." (If that's an accurate statement.) Rephrased to // Move any idle writers to entry_->readers. Should only be invoked when a // response is completely written and when ContainesOnlyIdleWriters() // returns true.
Next round of comments. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:39: DCHECK_EQ(next_state_, State::NONE); On 2017/05/31 19:21:26, shivanisha wrote: > On 2017/05/22 at 01:56:55, Randy Smith (Not in Mondays) wrote: > > On 2017/05/18 20:59:44, shivanisha wrote: > > > On 2017/05/18 at 01:02:55, Randy Smith (Not in Mondays) wrote: > > > > This (and the test above) seem wrong to me--if an active transaction can > > > remove itself while a network transaction is outstanding, the test above > should > > > be for if a network transaction is outstanding/the state machine is active. > > > > > > Ah, that's right. Changed the condition to next_state_ != State::NONE > > > > I'm not seeing the change (looking at PS6->8 diff)? > > It is present in PS8. Ah, sorry, was looking at the DCHECK. Sorry. I do think the DCHECK is basically redundant now, and I'd suggest removing it, but up to you. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:154: SetIdleWritersFailState(error); On 2017/05/31 19:21:26, shivanisha wrote: > On 2017/05/24 at 23:09:18, Randy Smith (Not in Mondays) wrote: > > On 2017/05/19 13:49:57, shivanisha wrote: > > > On 2017/05/18 at 20:59:44, shivanisha wrote: > > > > On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > > > > > Why have this logic in the consumers (HC::Ts) instead of in this class? > The > > > interface would be simpler if this class just got into a state where calls > to > > > Read() would just return |error|. That means that HC::Ts might read from > the > > > cache for a while before hitting the error, but I think that's a good thing? > > > > They might only end up needing what was in the cache. > > > > > > > > Its possible that entry is destroyed and then only HC::T is left to take > care > > > of a future read. > > > > > > Or we can doom this entry but let these transaction still associated so that > > > Active Entry is not destroyed? > > > > Yeah, that was the direction I think I was gesturing in. But I don't think > it's worth a lot of complex coding/changing current implementation a lot. Could > you sketch out to me the path which would resulting in destroying the entry > while there are HC::Ts that were once associated with it? > > If there is a failure then waiting_for_read_ transactions' callbacks are invoked > with the error, idle transactions have a state set which will be checked in > their future Read calls and Writers is then empty. Writers will then invoke > cache_->DoneWritingToEntry with success as false leading to ProcessEntryFailure > being called which will destroy the entry and restart any add_to_entry_queue > transactions. Makes sense. Ok, I'm slightly in favor at the architectural level of dooming the entry but not destroying it while there are still associated transactions--the AE is the coordination point, so it makes sense to me for it to hold the error state. Having said that, as I say above I don't think it's worth a lot of code changes, so feel free to do that or not as you prefer. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.h:81: void SetExclusive() { is_exclusive_ = true; } On 2017/05/31 19:21:26, shivanisha wrote: > On 2017/05/22 at 01:56:55, Randy Smith (Not in Mondays) wrote: > > On 2017/05/18 01:02:56, Randy Smith (Not in Mondays) wrote: > > > What happens if this is called with a lot of transactions on the object? It > > > would seem like more handling than just setting the bit would be necessary; > can > > > you give me a sense as to why that isn't true? > > > > Ping? > > Added a dcheck that only 0 or 1 transactions should be present when this is > called. SG. Just so I'm tracking, what's the use case for zero transactions? https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.h:216: CompletionCallback io_callback_; On 2017/05/31 19:21:26, shivanisha wrote: > On 2017/05/22 at 01:56:56, Randy Smith (Not in Mondays) wrote: > > On 2017/05/18 01:02:56, Randy Smith (Not in Mondays) wrote: > > > Suggestion: In my ideal world this would be const, since it's initialized in > the > > > constructor and never touched after that. That might mean you need to > > > initialize it in the constructor data member list, which might mean you need > to > > > put it after WeakPtrFactory, or maybe cast? But I think marking this const > > > would be a useful signal to future readers. > > > > > > An alternative would be to actually just replicate the Bind() call in all > three > > > places where this is used, which I'd be completely fine with but I realize > is > > > inconsistent with usage elsewhere in the cache. More code, but IMO more > > > readable code (since the reader doesn't have to lookup the io_callback_ > > > initialization). > > > > Ping? > > It seems nothing should come after WeakPtrFactory in the class as per > https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?q=WeakPtrFactory&... > > Adding initialization on all 3 call sites. If you're doing the initialization at the call sites you can use a local variable or do it inline; no need for this declaration. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:42: active_transaction_ = transaction; DCHECK_EQ(nullptr, active_transaction_.get())? (Suggestion) https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:94: void HttpCache::Writers::RemoveTransaction(Transaction* transaction) { Suggestion: DCHECK that the transaction was found in one of the queues? (I.e. that we weren't passed a random transaction.) https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:95: if (!transaction) What's the use case for calling RemoveTransaction(nullptr)? It seems a bit weird to have the responsibility of catching this case be in the Writers class. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:156: return !is_exclusive_ && !network_read_only_; What's the use case for adding writers when |all_writers_.empty() && network_read_only_|? https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:169: ResetStateForEmptyWriters(); Shouldn't this already have happened if all_writers_ is empty? Seems like it should be a DCHECK(), not a call. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:238: read_buf_ = NULL; // Release the buffer before invoking the callback. Does it matter whether we release the buffer before or after invoking the callback? (Alternative phrasing: Suggestion: Remove the comment?) https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:261: void HttpCache::Writers::OnNetworkReadFailure(int result) { Why does this function take a |result| argument? https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:263: // If no consumer, invoke entry processing here itself. This comment doesn't seem easily relatable to the code; specifically, I don't see any conditional that I can match to "if no consumer". Could you clarify it? https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:280: int HttpCache::Writers::WriteToEntry(int index, Given that this is a private function only called above, I'd suggest hardcoding the kResponseContentIndex inside the function rather than taking it as an argument. (Unless you have an anticipated use case in your next couple of CLs that would use the argument functionality.) https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:332: DLOG(ERROR) << "failed to write response info to cache"; What state does this leave the cache in? If it's not a consistent state (e.g. and earlier or later write of headers would contradict the amount of data on disk), I'm inclined to think we should doom the entry. Of course, that's assume the disk isn't just refusing operations :-J. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:341: // the entry as truncated. I don't see (here or in OnNetworkReadFailure()) an attempt to mark the entry as truncated? https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:354: } Why not combine these two conditionals? |completed| doesn't look like it's used anywhere else. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:364: // response completion. What gets in the way of doing this now? https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:381: it++) { nit, suggestion: Why not use the new C++ range syntax? (for (auto& waiting_transaction: waiting_for_read_))? https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:387: for (auto it = waiting_for_read_.begin(); it != waiting_for_read_.end(); I've read over this code a couple of times, and I'm failing to understand the need for the two nested for loops. Isn't the algorithm you want: * For each waiting transaction: ** local_result = result ** If success: *** local_result = min(result, read_buf_len) *** Copy local_result bytes into the read buffer. ** If failure, erase the transaction from the list. ** Post the callback? https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.h:26: // |*entry| and |*cache| should always be valid when accessed from Writers. I think of comments as being directions to the consumer as to what rules they have to follow for the class to behave properly for them. The consumer of HC::W doesn't know when entry/cache are going to be accessed. So I'd phrase this as "The consumer must guarantee that |*entry| and |*cache| outlive this object." https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.h:34: // via the |callback|. nit, suggestion: I think there's a bit of implementation detail (the waiting queue, the specification of the reason for the IO_PENDING) leaking into this interface contract. From the caller's perspective, it doesn't matter whether the sync/async return is because there's already a read in progress or because the network read is invoked and *it's* async. Of course, the purpose of this class is coordinating among multiple readers, so that can/should be mentioned. I'd suggest something like "Retrieves data from the network transaction associated with the Writers object. This may be done directly (via a network read into |*buf->data()|) or indirectly (by copying from another transactions buffer into |*buf->data()| on network read completion) depending on whether or not a read is current in progress. May return the result synchronously or return ERR_IO_PENDING: if ERR_IO_PENDING is returned, |callback| will be run to inform the consumer of the result of the Read()." https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.h:53: // point and it should invoke RemoveTransaction() during its destruction. This leaves me a little confused as to whether it's legal to pass a network_transaction if this isn't the first AddTransaction() call, and what happens if that is done. Would you clarify the comment? https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.h:94: // not currently in progress. How will the caller know that reading is not currently in progress? More generally, I'm trying to wrap my head around the use case for this. Looking at HC::T, it looks like we truncate cached data after we receive the response headers. But that means it would happen in the validating transaction, not when we're sharing access, or maybe on the addition of the first writer & the network transaction to the class. Would it make sense to combine those functionalities? https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.h:97: LoadState GetWriterLoadState(); This currently requires a network transaction to be attached to the structure. Given that the structure seems to allow a null network transaction (during initialization or in some corner cases) shouldn't there be a (consumer oriented) comment here saying when it can be called? Or the routine should avoid requiring network_transaction_ set?
Next round of comments.
On 2017/06/06 12:55:33, Randy Smith (Not in Mondays) wrote: > Next round of comments. Sorry, connectivity glitch, please ignore. c#45 is my "Next round of comments."
Patchset #11 (id:280001) has been deleted
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #11 (id:300001) has been deleted
Patchset #11 (id:320001) has been deleted
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, Thanks! Has the latest feedback addressed. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:39: DCHECK_EQ(next_state_, State::NONE); On 2017/06/06 at 12:54:57, Randy Smith (Not in Mondays) wrote: > On 2017/05/31 19:21:26, shivanisha wrote: > > On 2017/05/22 at 01:56:55, Randy Smith (Not in Mondays) wrote: > > > On 2017/05/18 20:59:44, shivanisha wrote: > > > > On 2017/05/18 at 01:02:55, Randy Smith (Not in Mondays) wrote: > > > > > This (and the test above) seem wrong to me--if an active transaction can > > > > remove itself while a network transaction is outstanding, the test above > > should > > > > be for if a network transaction is outstanding/the state machine is active. > > > > > > > > Ah, that's right. Changed the condition to next_state_ != State::NONE > > > > > > I'm not seeing the change (looking at PS6->8 diff)? > > > > It is present in PS8. > > Ah, sorry, was looking at the DCHECK. Sorry. > > I do think the DCHECK is basically redundant now, and I'd suggest removing it, but up to you. Removed the dcheck. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:154: SetIdleWritersFailState(error); On 2017/06/06 at 12:54:57, Randy Smith (Not in Mondays) wrote: > On 2017/05/31 19:21:26, shivanisha wrote: > > On 2017/05/24 at 23:09:18, Randy Smith (Not in Mondays) wrote: > > > On 2017/05/19 13:49:57, shivanisha wrote: > > > > On 2017/05/18 at 20:59:44, shivanisha wrote: > > > > > On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > > > > > > Why have this logic in the consumers (HC::Ts) instead of in this class? > > The > > > > interface would be simpler if this class just got into a state where calls > > to > > > > Read() would just return |error|. That means that HC::Ts might read from > > the > > > > cache for a while before hitting the error, but I think that's a good thing? > > > > > > They might only end up needing what was in the cache. > > > > > > > > > > Its possible that entry is destroyed and then only HC::T is left to take > > care > > > > of a future read. > > > > > > > > Or we can doom this entry but let these transaction still associated so that > > > > Active Entry is not destroyed? > > > > > > Yeah, that was the direction I think I was gesturing in. But I don't think > > it's worth a lot of complex coding/changing current implementation a lot. Could > > you sketch out to me the path which would resulting in destroying the entry > > while there are HC::Ts that were once associated with it? > > > > If there is a failure then waiting_for_read_ transactions' callbacks are invoked > > with the error, idle transactions have a state set which will be checked in > > their future Read calls and Writers is then empty. Writers will then invoke > > cache_->DoneWritingToEntry with success as false leading to ProcessEntryFailure > > being called which will destroy the entry and restart any add_to_entry_queue > > transactions. > > Makes sense. Ok, I'm slightly in favor at the architectural level of dooming the entry but not destroying it while there are still associated transactions--the AE is the coordination point, so it makes sense to me for it to hold the error state. Having said that, as I say above I don't think it's worth a lot of code changes, so feel free to do that or not as you prefer. Changed the logic to keep this state in Writers itself as that seems simpler. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.h:81: void SetExclusive() { is_exclusive_ = true; } On 2017/06/06 at 12:54:57, Randy Smith (Not in Mondays) wrote: > On 2017/05/31 19:21:26, shivanisha wrote: > > On 2017/05/22 at 01:56:55, Randy Smith (Not in Mondays) wrote: > > > On 2017/05/18 01:02:56, Randy Smith (Not in Mondays) wrote: > > > > What happens if this is called with a lot of transactions on the object? It > > > > would seem like more handling than just setting the bit would be necessary; > > can > > > > you give me a sense as to why that isn't true? > > > > > > Ping? > > > > Added a dcheck that only 0 or 1 transactions should be present when this is > > called. > > SG. Just so I'm tracking, what's the use case for zero transactions? Actually, adding an is_exclusive argument to AddTransaction and removing this function. And the DCHECK basically says that this should be the first transaction added if is_exclusive is true. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.h:216: CompletionCallback io_callback_; On 2017/06/06 at 12:54:57, Randy Smith (Not in Mondays) wrote: > On 2017/05/31 19:21:26, shivanisha wrote: > > On 2017/05/22 at 01:56:56, Randy Smith (Not in Mondays) wrote: > > > On 2017/05/18 01:02:56, Randy Smith (Not in Mondays) wrote: > > > > Suggestion: In my ideal world this would be const, since it's initialized in > > the > > > > constructor and never touched after that. That might mean you need to > > > > initialize it in the constructor data member list, which might mean you need > > to > > > > put it after WeakPtrFactory, or maybe cast? But I think marking this const > > > > would be a useful signal to future readers. > > > > > > > > An alternative would be to actually just replicate the Bind() call in all > > three > > > > places where this is used, which I'd be completely fine with but I realize > > is > > > > inconsistent with usage elsewhere in the cache. More code, but IMO more > > > > readable code (since the reader doesn't have to lookup the io_callback_ > > > > initialization). > > > > > > Ping? > > > > It seems nothing should come after WeakPtrFactory in the class as per > > https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?q=WeakPtrFactory&... > > > > Adding initialization on all 3 call sites. > > If you're doing the initialization at the call sites you can use a local variable or do it inline; no need for this declaration. Removed the member variable. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:42: active_transaction_ = transaction; On 2017/06/06 at 12:54:57, Randy Smith (Not in Mondays) wrote: > DCHECK_EQ(nullptr, active_transaction_.get())? (Suggestion) done https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:94: void HttpCache::Writers::RemoveTransaction(Transaction* transaction) { On 2017/06/06 at 12:54:57, Randy Smith (Not in Mondays) wrote: > Suggestion: DCHECK that the transaction was found in one of the queues? (I.e. that we weren't passed a random transaction.) DCHECK(it != all_writers_.end()); below takes care of that. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:95: if (!transaction) On 2017/06/06 at 12:54:58, Randy Smith (Not in Mondays) wrote: > What's the use case for calling RemoveTransaction(nullptr)? It seems a bit weird to have the responsibility of catching this case be in the Writers class. Let's say active_transaction_ is null as it has been destroyed. Writers invokes cache_->DoneWritingToEntry with transaction as nullptr => RemoveTransaction(nullptr) https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:156: return !is_exclusive_ && !network_read_only_; On 2017/06/06 at 12:54:58, Randy Smith (Not in Mondays) wrote: > What's the use case for adding writers when |all_writers_.empty() && network_read_only_|? That shouldn't happen since network_read_only_ is false to start with and reset when writers is empty. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:169: ResetStateForEmptyWriters(); On 2017/06/06 at 12:54:57, Randy Smith (Not in Mondays) wrote: > Shouldn't this already have happened if all_writers_ is empty? Seems like it should be a DCHECK(), not a call. This basically resets some member variables and should be invoked when the all the transactions have been removed which is what is happening in the statements above. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:238: read_buf_ = NULL; // Release the buffer before invoking the callback. On 2017/06/06 at 12:54:58, Randy Smith (Not in Mondays) wrote: > Does it matter whether we release the buffer before or after invoking the callback? (Alternative phrasing: Suggestion: Remove the comment?) From what I understand its important to release before since its a scoped_refptr and can only be used by one layer at a time and now we want the consumer layer to use it. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:261: void HttpCache::Writers::OnNetworkReadFailure(int result) { On 2017/06/06 at 12:54:57, Randy Smith (Not in Mondays) wrote: > Why does this function take a |result| argument? A follow up CL will use result to invoke ProcessFailure. Since I am still working on whether to invoke it here directly from here or through cache_->DoneWithEntry, I am not adding it yet in this CL. Added a TODO for the same. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:263: // If no consumer, invoke entry processing here itself. On 2017/06/06 at 12:54:57, Randy Smith (Not in Mondays) wrote: > This comment doesn't seem easily relatable to the code; specifically, I don't see any conditional that I can match to "if no consumer". Could you clarify it? Yes, removed it. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:280: int HttpCache::Writers::WriteToEntry(int index, On 2017/06/06 at 12:54:58, Randy Smith (Not in Mondays) wrote: > Given that this is a private function only called above, I'd suggest hardcoding the kResponseContentIndex inside the function rather than taking it as an argument. (Unless you have an anticipated use case in your next couple of CLs that would use the argument functionality.) done https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:332: DLOG(ERROR) << "failed to write response info to cache"; On 2017/06/06 at 12:54:57, Randy Smith (Not in Mondays) wrote: > What state does this leave the cache in? If it's not a consistent state (e.g. and earlier or later write of headers would contradict the amount of data on disk), I'm inclined to think we should doom the entry. Of course, that's assume the disk isn't just refusing operations :-J. done https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:341: // the entry as truncated. On 2017/06/06 at 12:54:57, Randy Smith (Not in Mondays) wrote: > I don't see (here or in OnNetworkReadFailure()) an attempt to mark the entry as truncated? OnNetworkReadFailure() => DoneWithEntry() => Invokes truncation code. I am leaving the decision to do it directly here or through DoneWithEntry() for the follow up CL which integrates it with HttpCache. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:354: } On 2017/06/06 at 12:54:58, Randy Smith (Not in Mondays) wrote: > Why not combine these two conditionals? |completed| doesn't look like it's used anywhere else. done https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:364: // response completion. On 2017/06/06 at 12:54:58, Randy Smith (Not in Mondays) wrote: > What gets in the way of doing this now? Messes up with the assertions in HttpCache layer so can add it only after integration with both HC::T and HC. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:381: it++) { On 2017/06/06 at 12:54:57, Randy Smith (Not in Mondays) wrote: > nit, suggestion: Why not use the new C++ range syntax? (for (auto& waiting_transaction: waiting_for_read_))? done https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:387: for (auto it = waiting_for_read_.begin(); it != waiting_for_read_.end(); On 2017/06/06 at 12:54:58, Randy Smith (Not in Mondays) wrote: > I've read over this code a couple of times, and I'm failing to understand the need for the two nested for loops. Isn't the algorithm you want: > * For each waiting transaction: > ** local_result = result > ** If success: > *** local_result = min(result, read_buf_len) > *** Copy local_result bytes into the read buffer. > ** If failure, erase the transaction from the list. > ** Post the callback? Oops, corrected. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.h:26: // |*entry| and |*cache| should always be valid when accessed from Writers. On 2017/06/06 at 12:54:58, Randy Smith (Not in Mondays) wrote: > I think of comments as being directions to the consumer as to what rules they have to follow for the class to behave properly for them. The consumer of HC::W doesn't know when entry/cache are going to be accessed. So I'd phrase this as "The consumer must guarantee that |*entry| and |*cache| outlive this object." done https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.h:34: // via the |callback|. On 2017/06/06 at 12:54:58, Randy Smith (Not in Mondays) wrote: > nit, suggestion: I think there's a bit of implementation detail (the waiting queue, the specification of the reason for the IO_PENDING) leaking into this interface contract. From the caller's perspective, it doesn't matter whether the sync/async return is because there's already a read in progress or because the network read is invoked and *it's* async. Of course, the purpose of this class is coordinating among multiple readers, so that can/should be mentioned. I'd suggest something like > > "Retrieves data from the network transaction associated with the Writers object. This may be done directly (via a network read into |*buf->data()|) or indirectly (by copying from another transactions buffer into |*buf->data()| on network read completion) depending on whether or not a read is current in progress. May return the result synchronously or return ERR_IO_PENDING: if ERR_IO_PENDING is returned, |callback| will be run to inform the consumer of the result of the Read()." done https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.h:53: // point and it should invoke RemoveTransaction() during its destruction. On 2017/06/06 at 12:54:58, Randy Smith (Not in Mondays) wrote: > This leaves me a little confused as to whether it's legal to pass a network_transaction if this isn't the first AddTransaction() call, and what happens if that is done. Would you clarify the comment? Rephrased https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.h:94: // not currently in progress. On 2017/06/06 at 12:54:58, Randy Smith (Not in Mondays) wrote: > How will the caller know that reading is not currently in progress? > > More generally, I'm trying to wrap my head around the use case for this. Looking at HC::T, it looks like we truncate cached data after we receive the response headers. But that means it would happen in the validating transaction, not when we're sharing access, or maybe on the addition of the first writer & the network transaction to the class. Would it make sense to combine those functionalities? As a parallel with HC::T states this is not equivalent to STATE_TRUNCATE_CACHED_DATA* which happens after we receive response headers but this is equivalent to STATE_CACHE_WRITE_TRUNCATED_RESPONSE* which might happen when transaction responsible for writing the data is destroyed. See AddTruncatedFlag(). Removing the comment to make it simpler. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.h:97: LoadState GetWriterLoadState(); On 2017/06/06 at 12:54:58, Randy Smith (Not in Mondays) wrote: > This currently requires a network transaction to be attached to the structure. Given that the structure seems to allow a null network transaction (during initialization or in some corner cases) shouldn't there be a (consumer oriented) comment here saying when it can be called? Or the routine should avoid requiring network_transaction_ set? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:154: SetIdleWritersFailState(error); On 2017/06/07 at 15:19:18, shivanisha wrote: > On 2017/06/06 at 12:54:57, Randy Smith (Not in Mondays) wrote: > > On 2017/05/31 19:21:26, shivanisha wrote: > > > On 2017/05/24 at 23:09:18, Randy Smith (Not in Mondays) wrote: > > > > On 2017/05/19 13:49:57, shivanisha wrote: > > > > > On 2017/05/18 at 20:59:44, shivanisha wrote: > > > > > > On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > > > > > > > Why have this logic in the consumers (HC::Ts) instead of in this class? > > > The > > > > > interface would be simpler if this class just got into a state where calls > > > to > > > > > Read() would just return |error|. That means that HC::Ts might read from > > > the > > > > > cache for a while before hitting the error, but I think that's a good thing? > > > > > > > > They might only end up needing what was in the cache. > > > > > > > > > > > > Its possible that entry is destroyed and then only HC::T is left to take > > > care > > > > > of a future read. > > > > > > > > > > Or we can doom this entry but let these transaction still associated so that > > > > > Active Entry is not destroyed? > > > > > > > > Yeah, that was the direction I think I was gesturing in. But I don't think > > > it's worth a lot of complex coding/changing current implementation a lot. Could > > > you sketch out to me the path which would resulting in destroying the entry > > > while there are HC::Ts that were once associated with it? > > > > > > If there is a failure then waiting_for_read_ transactions' callbacks are invoked > > > with the error, idle transactions have a state set which will be checked in > > > their future Read calls and Writers is then empty. Writers will then invoke > > > cache_->DoneWritingToEntry with success as false leading to ProcessEntryFailure > > > being called which will destroy the entry and restart any add_to_entry_queue > > > transactions. > > > > Makes sense. Ok, I'm slightly in favor at the architectural level of dooming the entry but not destroying it while there are still associated transactions--the AE is the coordination point, so it makes sense to me for it to hold the error state. Having said that, as I say above I don't think it's worth a lot of code changes, so feel free to do that or not as you prefer. > > Changed the logic to keep this state in Writers itself as that seems simpler. Actually while working on the follow up CL, a use case came up because of which the earlier design would be preferable which sets the state in the individual transactions and not in the Writers class. So, let's say there is a cache write failure. Now we want a fail state for future Reads for all idle transactions but not for the transaction that actually had the cache write failure since it can continue to read from the network through writers. So making that change in the follow up CL that integrates this class with HC::T and HC.
The basic functionality is looking good modulo integration (it's hard to evaluate interface contracts without seeing the consumer code). I'd like to hold off on evaluating the test code until after Josh does his review of the main functionality, since that could change architecture and hence tests. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:154: SetIdleWritersFailState(error); On 2017/06/08 13:38:41, shivanisha wrote: > On 2017/06/07 at 15:19:18, shivanisha wrote: > > On 2017/06/06 at 12:54:57, Randy Smith (Not in Mondays) wrote: > > > On 2017/05/31 19:21:26, shivanisha wrote: > > > > On 2017/05/24 at 23:09:18, Randy Smith (Not in Mondays) wrote: > > > > > On 2017/05/19 13:49:57, shivanisha wrote: > > > > > > On 2017/05/18 at 20:59:44, shivanisha wrote: > > > > > > > On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > > > > > > > > Why have this logic in the consumers (HC::Ts) instead of in this > class? > > > > The > > > > > > interface would be simpler if this class just got into a state where > calls > > > > to > > > > > > Read() would just return |error|. That means that HC::Ts might read > from > > > > the > > > > > > cache for a while before hitting the error, but I think that's a good > thing? > > > > > > > > > > They might only end up needing what was in the cache. > > > > > > > > > > > > > > Its possible that entry is destroyed and then only HC::T is left to > take > > > > care > > > > > > of a future read. > > > > > > > > > > > > Or we can doom this entry but let these transaction still associated > so that > > > > > > Active Entry is not destroyed? > > > > > > > > > > Yeah, that was the direction I think I was gesturing in. But I don't > think > > > > it's worth a lot of complex coding/changing current implementation a lot. > Could > > > > you sketch out to me the path which would resulting in destroying the > entry > > > > while there are HC::Ts that were once associated with it? > > > > > > > > If there is a failure then waiting_for_read_ transactions' callbacks are > invoked > > > > with the error, idle transactions have a state set which will be checked > in > > > > their future Read calls and Writers is then empty. Writers will then > invoke > > > > cache_->DoneWritingToEntry with success as false leading to > ProcessEntryFailure > > > > being called which will destroy the entry and restart any > add_to_entry_queue > > > > transactions. > > > > > > Makes sense. Ok, I'm slightly in favor at the architectural level of > dooming the entry but not destroying it while there are still associated > transactions--the AE is the coordination point, so it makes sense to me for it > to hold the error state. Having said that, as I say above I don't think it's > worth a lot of code changes, so feel free to do that or not as you prefer. > > > > Changed the logic to keep this state in Writers itself as that seems simpler. > > Actually while working on the follow up CL, a use case came up because of which > the earlier design would be preferable which sets the state in the individual > transactions and not in the Writers class. So, let's say there is a cache write > failure. Now we want a fail state for future Reads for all idle transactions but > not for the transaction that actually had the cache write failure since it can > continue to read from the network through writers. So making that change in the > follow up CL that integrates this class with HC::T and HC. Accepted if that's how you decide that's the best way to handle it, but in the case you describe, the cache write is happening on behalf of the active transaction. So you could set the fail state on the writers class for future reads, while returning success for the current transaction. I think that's conceptually clean--all future reads bounce, but Read() has already been called for the transaction that you want to return successfully. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:95: if (!transaction) On 2017/06/07 15:19:18, shivanisha wrote: > On 2017/06/06 at 12:54:58, Randy Smith (Not in Mondays) wrote: > > What's the use case for calling RemoveTransaction(nullptr)? It seems a bit > weird to have the responsibility of catching this case be in the Writers class. > > Let's say active_transaction_ is null as it has been destroyed. > Writers invokes cache_->DoneWritingToEntry with transaction as nullptr => > RemoveTransaction(nullptr) I guess I'll put this off until the integration CL--it's hard to judge an interface contract without seeing it being used. But I am still confused; specifically, DoneWritingToEntry() doesn't currently take a transaction, so the cache can't call back into RemoveTransaction() with it. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:169: ResetStateForEmptyWriters(); On 2017/06/07 15:19:18, shivanisha wrote: > On 2017/06/06 at 12:54:57, Randy Smith (Not in Mondays) wrote: > > Shouldn't this already have happened if all_writers_ is empty? Seems like it > should be a DCHECK(), not a call. > > This basically resets some member variables and should be invoked when the all > the transactions have been removed which is what is happening in the statements > above. This makes the interface contract for ProcessWaitingForReadTransaction() a bit messy, in that it's now implicitly the responsibility of the caller of ProcessWaitingForReadTransactions() to call ResetStateForEmptyWriters(). Would you be willing to move the state reset to the end of PWFRT() outside the loop? That's an extra check in the failure case, but beyond that matches the performance of the current implementation, and leaves the responsibilities cleaner. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:238: read_buf_ = NULL; // Release the buffer before invoking the callback. On 2017/06/07 15:19:19, shivanisha wrote: > On 2017/06/06 at 12:54:58, Randy Smith (Not in Mondays) wrote: > > Does it matter whether we release the buffer before or after invoking the > callback? (Alternative phrasing: Suggestion: Remove the comment?) > > From what I understand its important to release before since its a scoped_refptr > and can only be used by one layer at a time and now we want the consumer layer > to use it. It's true that it can be only used by one layer at a time, but the reference is for keeping it alive, not for using it. For instance, there are multiple references to the buffer when it's passed to the network transaction, but the network transaction is still the only layer that's allowed to use it. I believe the only effect of releasing the buffer after the callback instead of before would be that the buffer deletion would occur at that line, rather than in the callback. Which, who knows, might make some future bug a bit harder to debug, so I certainly don't object to releasing the buffer before the callback call. I just don't think that there's any requirement to do so for code correctness, performance, or cleanliness, so I think the comment is misleading (in its implication that it's important that it be released first). But up to you--I'm just explaining my reasoning for clarity and possibly interest, not because I feel strongly. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:341: // the entry as truncated. On 2017/06/07 15:19:19, shivanisha wrote: > On 2017/06/06 at 12:54:57, Randy Smith (Not in Mondays) wrote: > > I don't see (here or in OnNetworkReadFailure()) an attempt to mark the entry > as truncated? > > OnNetworkReadFailure() => DoneWithEntry() => Invokes truncation code. I am > leaving the decision to do it directly here or through DoneWithEntry() for the > follow up CL which integrates it with HttpCache. Gotchat, and that's fine. But I find the comment confusing as written; if you leave the code like this could you add something about where the truncation happens (e.g. "... truncated (in OnNetworkReadFailure => DoneWithEntry())")? https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.cc:45: active_transaction_ = transaction; Worth a DCHECK that transaction is in all_writers_? https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.cc:52: if (rv == ERR_IO_PENDING) { nit, suggestion: No need for curly braces. https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.cc:279: int current_size = entry_->disk_entry->GetDataSize(kResponseContentIndex); Why is this accurate as the offset to which to write in the cache in a partial case? Couldn't a partial read be anywhere in the entry? https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.h:54: // true, it makes writing an exclusine operation implying that Writers can nit: exclusive https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.h:60: bool is_exclusive); The current implementation requires the consumer to take responsibility to not call in with an exclusive bit set if that's not legal. My instinct is that it might make more sense to just have AddTransaction return a success flag, and bounce the add if is_exclusive isn't legal. Whether this is the right idea or not depends heavily on how the function is used, so I'll defer to your judgement and/or re-raise the issue in the integration CL, but I wanted to call out the possibility of the slightly different interface contract now. https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.h:115: FAIL_READ My personal preference would be to keep the invariant that states in State correspond to sub-functions of DoLoop() and the semantics of those states are implemented in the subfunctions. I'd suggest turning this into a boolean instead of a state.
Responding to some of the comments that did not need a code change. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:154: SetIdleWritersFailState(error); On 2017/06/08 at 17:10:00, Randy Smith (Not in Mondays) wrote: > On 2017/06/08 13:38:41, shivanisha wrote: > > On 2017/06/07 at 15:19:18, shivanisha wrote: > > > On 2017/06/06 at 12:54:57, Randy Smith (Not in Mondays) wrote: > > > > On 2017/05/31 19:21:26, shivanisha wrote: > > > > > On 2017/05/24 at 23:09:18, Randy Smith (Not in Mondays) wrote: > > > > > > On 2017/05/19 13:49:57, shivanisha wrote: > > > > > > > On 2017/05/18 at 20:59:44, shivanisha wrote: > > > > > > > > On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > > > > > > > > > Why have this logic in the consumers (HC::Ts) instead of in this > > class? > > > > > The > > > > > > > interface would be simpler if this class just got into a state where > > calls > > > > > to > > > > > > > Read() would just return |error|. That means that HC::Ts might read > > from > > > > > the > > > > > > > cache for a while before hitting the error, but I think that's a good > > thing? > > > > > > > > > > > > They might only end up needing what was in the cache. > > > > > > > > > > > > > > > > Its possible that entry is destroyed and then only HC::T is left to > > take > > > > > care > > > > > > > of a future read. > > > > > > > > > > > > > > Or we can doom this entry but let these transaction still associated > > so that > > > > > > > Active Entry is not destroyed? > > > > > > > > > > > > Yeah, that was the direction I think I was gesturing in. But I don't > > think > > > > > it's worth a lot of complex coding/changing current implementation a lot. > > Could > > > > > you sketch out to me the path which would resulting in destroying the > > entry > > > > > while there are HC::Ts that were once associated with it? > > > > > > > > > > If there is a failure then waiting_for_read_ transactions' callbacks are > > invoked > > > > > with the error, idle transactions have a state set which will be checked > > in > > > > > their future Read calls and Writers is then empty. Writers will then > > invoke > > > > > cache_->DoneWritingToEntry with success as false leading to > > ProcessEntryFailure > > > > > being called which will destroy the entry and restart any > > add_to_entry_queue > > > > > transactions. > > > > > > > > Makes sense. Ok, I'm slightly in favor at the architectural level of > > dooming the entry but not destroying it while there are still associated > > transactions--the AE is the coordination point, so it makes sense to me for it > > to hold the error state. Having said that, as I say above I don't think it's > > worth a lot of code changes, so feel free to do that or not as you prefer. > > > > > > Changed the logic to keep this state in Writers itself as that seems simpler. > > > > Actually while working on the follow up CL, a use case came up because of which > > the earlier design would be preferable which sets the state in the individual > > transactions and not in the Writers class. So, let's say there is a cache write > > failure. Now we want a fail state for future Reads for all idle transactions but > > not for the transaction that actually had the cache write failure since it can > > continue to read from the network through writers. So making that change in the > > follow up CL that integrates this class with HC::T and HC. > > Accepted if that's how you decide that's the best way to handle it, but in the case you describe, the cache write is happening on behalf of the active transaction. So you could set the fail state on the writers class for future reads, while returning success for the current transaction. I think that's conceptually clean--all future reads bounce, but Read() has already been called for the transaction that you want to return successfully. We actually want future reads on the active_transaction_ to go through as well. Since network_read_only_ will be set to true, these future reads will only read from the network. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:95: if (!transaction) On 2017/06/08 at 17:10:00, Randy Smith (Not in Mondays) wrote: > On 2017/06/07 15:19:18, shivanisha wrote: > > On 2017/06/06 at 12:54:58, Randy Smith (Not in Mondays) wrote: > > > What's the use case for calling RemoveTransaction(nullptr)? It seems a bit > > weird to have the responsibility of catching this case be in the Writers class. > > > > Let's say active_transaction_ is null as it has been destroyed. > > Writers invokes cache_->DoneWritingToEntry with transaction as nullptr => > > RemoveTransaction(nullptr) > > I guess I'll put this off until the integration CL--it's hard to judge an interface contract without seeing it being used. But I am still confused; specifically, DoneWritingToEntry() doesn't currently take a transaction, so the cache can't call back into RemoveTransaction() with it. DoneWritingToEntry() takes the transaction in the parallel validation CL https://codereview.chromium.org/2721933002/ https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:169: ResetStateForEmptyWriters(); On 2017/06/08 at 17:10:00, Randy Smith (Not in Mondays) wrote: > On 2017/06/07 15:19:18, shivanisha wrote: > > On 2017/06/06 at 12:54:57, Randy Smith (Not in Mondays) wrote: > > > Shouldn't this already have happened if all_writers_ is empty? Seems like it > > should be a DCHECK(), not a call. > > > > This basically resets some member variables and should be invoked when the all > > the transactions have been removed which is what is happening in the statements > > above. > > This makes the interface contract for ProcessWaitingForReadTransaction() a bit messy, in that it's now implicitly the responsibility of the caller of ProcessWaitingForReadTransactions() to call ResetStateForEmptyWriters(). Would you be willing to move the state reset to the end of PWFRT() outside the loop? That's an extra check in the failure case, but beyond that matches the performance of the current implementation, and leaves the responsibilities cleaner. ResetStateForEmptyWriters() is not really related to ProcessWaitingForReadTransaction(). ProcessWaitingForReadTransaction takes care of processing the IO callbacks of waiting transactions and ResetStateForEmptyWriters is responsible for resetting state in other cases as well and not just when waiting_for_read is processed. So I see them as separate responsibilities and here ProcessFailure() takes care of both responsibilities. https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.cc:279: int current_size = entry_->disk_entry->GetDataSize(kResponseContentIndex); On 2017/06/08 at 17:10:01, Randy Smith (Not in Mondays) wrote: > Why is this accurate as the offset to which to write in the cache in a partial case? Couldn't a partial read be anywhere in the entry? WriteToEntry => partial->CacheWrite which does not take the offset.
https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_tr... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_tr... net/http/http_cache_transaction.h:170: RequestPriority priority() { return priority_; } this should be a const function https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.cc:83: DCHECK_EQ(all_writers_.size(), (size_t)1); DCHECK_EQ(1u, all_writers_.size()); DCHECK's crash message is written such that the expected value is the first argument. https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.cc:92: DCHECK(network_transaction_); I'd prefer we moved this to the top of the function: DCHECK(network_transaction_ || network_transaction); https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.cc:99: if (!transaction) Can we instead DCHECK(transaction)? https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.cc:109: PriorityChanged(); Perhaps rename to UpdatePriority since the priority may or may not have changed? https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.cc:120: waiting_for_read_.erase(waiting_it); Hmm, is the iterator still safe to increment after calling erase? Do we have a test covering the case of RemoveTransaction when there are multiple transactions? https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.cc:146: // Should be invoked after |waiting_for_read_| transactions and extra space between transactions and and https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.cc:151: entry_->readers.insert(idle_writer); I think at this point we should treat ActiveEntry as a class instead of a struct. Directly injecting into readers here means that there are now two files mutating ActiveEntries. For now, can you add a function to HttpCache for reader insertion? https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.h:7: #include <list> newline above https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.h:9: #include "base/memory/weak_ptr.h" newline above https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.h:49: // Adds an HttpCache::Transaction to Writers and if its the first transaction s/its/it's/ https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.h:115: FAIL_READ On 2017/06/08 17:10:01, Randy Smith (Not in Mondays) wrote: > My personal preference would be to keep the invariant that states in State > correspond to sub-functions of DoLoop() and the semantics of those states are > implemented in the subfunctions. I'd suggest turning this into a boolean > instead of a state. +1 https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.h:149: Remove newline? https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.h:226: #endif // NET_HTTP_HTTP_CACHE_WRITERS_H_ newline above https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:20: class HttpCache::WritersTest { Since every test uses this class, it seems like this ought to be a test fixture class. https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:215: } I'd like to see tests for all of the public API in this file, with lots of tests for behavior of multiple writers with writers/readers coming and going.
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patch 12 addresses Randy's rest of the feedback. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:238: read_buf_ = NULL; // Release the buffer before invoking the callback. On 2017/06/08 at 17:10:00, Randy Smith (Not in Mondays) wrote: > On 2017/06/07 15:19:19, shivanisha wrote: > > On 2017/06/06 at 12:54:58, Randy Smith (Not in Mondays) wrote: > > > Does it matter whether we release the buffer before or after invoking the > > callback? (Alternative phrasing: Suggestion: Remove the comment?) > > > > From what I understand its important to release before since its a scoped_refptr > > and can only be used by one layer at a time and now we want the consumer layer > > to use it. > > It's true that it can be only used by one layer at a time, but the reference is for keeping it alive, not for using it. For instance, there are multiple references to the buffer when it's passed to the network transaction, but the network transaction is still the only layer that's allowed to use it. > > I believe the only effect of releasing the buffer after the callback instead of before would be that the buffer deletion would occur at that line, rather than in the callback. Which, who knows, might make some future bug a bit harder to debug, so I certainly don't object to releasing the buffer before the callback call. I just don't think that there's any requirement to do so for code correctness, performance, or cleanliness, so I think the comment is misleading (in its implication that it's important that it be released first). > > But up to you--I'm just explaining my reasoning for clarity and possibly interest, not because I feel strongly. Removed the comment. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:341: // the entry as truncated. On 2017/06/08 at 17:10:00, Randy Smith (Not in Mondays) wrote: > On 2017/06/07 15:19:19, shivanisha wrote: > > On 2017/06/06 at 12:54:57, Randy Smith (Not in Mondays) wrote: > > > I don't see (here or in OnNetworkReadFailure()) an attempt to mark the entry > > as truncated? > > > > OnNetworkReadFailure() => DoneWithEntry() => Invokes truncation code. I am > > leaving the decision to do it directly here or through DoneWithEntry() for the > > follow up CL which integrates it with HttpCache. > > Gotchat, and that's fine. But I find the comment confusing as written; if you leave the code like this could you add something about where the truncation happens (e.g. "... truncated (in OnNetworkReadFailure => DoneWithEntry())")? Added in the comment that the truncation attempt will be in OnNetworkReadFailure https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.cc:45: active_transaction_ = transaction; On 2017/06/08 at 17:10:01, Randy Smith (Not in Mondays) wrote: > Worth a DCHECK that transaction is in all_writers_? done https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.cc:52: if (rv == ERR_IO_PENDING) { On 2017/06/08 at 17:10:01, Randy Smith (Not in Mondays) wrote: > nit, suggestion: No need for curly braces. removed https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.h:54: // true, it makes writing an exclusine operation implying that Writers can On 2017/06/08 at 17:10:01, Randy Smith (Not in Mondays) wrote: > nit: exclusive done https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.h:60: bool is_exclusive); On 2017/06/08 at 17:10:01, Randy Smith (Not in Mondays) wrote: > The current implementation requires the consumer to take responsibility to not call in with an exclusive bit set if that's not legal. My instinct is that it might make more sense to just have AddTransaction return a success flag, and bounce the add if is_exclusive isn't legal. Whether this is the right idea or not depends heavily on how the function is used, so I'll defer to your judgement and/or re-raise the issue in the integration CL, but I wanted to call out the possibility of the slightly different interface contract now. ok, let's come back to this in the integration CL https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.h:115: FAIL_READ On 2017/06/08 at 17:10:01, Randy Smith (Not in Mondays) wrote: > My personal preference would be to keep the invariant that states in State correspond to sub-functions of DoLoop() and the semantics of those states are implemented in the subfunctions. I'd suggest turning this into a boolean instead of a state. Deferring this comment since the follow up CL anyways removed this state.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm happy with the non-test code in this CL, modulo WriteToEntry(offset, ...) documentation/elimination. It sounds like Josh is requesting fairly large changes (additions) to the test file, so I'm going to hold off on my review of that file until that issue is resolved. https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/160001/net/http/http_cache_wr... net/http/http_cache_writers.cc:154: SetIdleWritersFailState(error); On 2017/06/08 17:55:56, shivanisha wrote: > On 2017/06/08 at 17:10:00, Randy Smith (Not in Mondays) wrote: > > On 2017/06/08 13:38:41, shivanisha wrote: > > > On 2017/06/07 at 15:19:18, shivanisha wrote: > > > > On 2017/06/06 at 12:54:57, Randy Smith (Not in Mondays) wrote: > > > > > On 2017/05/31 19:21:26, shivanisha wrote: > > > > > > On 2017/05/24 at 23:09:18, Randy Smith (Not in Mondays) wrote: > > > > > > > On 2017/05/19 13:49:57, shivanisha wrote: > > > > > > > > On 2017/05/18 at 20:59:44, shivanisha wrote: > > > > > > > > > On 2017/05/18 at 01:02:54, Randy Smith (Not in Mondays) wrote: > > > > > > > > > > Why have this logic in the consumers (HC::Ts) instead of in > this > > > class? > > > > > > The > > > > > > > > interface would be simpler if this class just got into a state > where > > > calls > > > > > > to > > > > > > > > Read() would just return |error|. That means that HC::Ts might > read > > > from > > > > > > the > > > > > > > > cache for a while before hitting the error, but I think that's a > good > > > thing? > > > > > > > > > > > > > > They might only end up needing what was in the cache. > > > > > > > > > > > > > > > > > > Its possible that entry is destroyed and then only HC::T is left > to > > > take > > > > > > care > > > > > > > > of a future read. > > > > > > > > > > > > > > > > Or we can doom this entry but let these transaction still > associated > > > so that > > > > > > > > Active Entry is not destroyed? > > > > > > > > > > > > > > Yeah, that was the direction I think I was gesturing in. But I > don't > > > think > > > > > > it's worth a lot of complex coding/changing current implementation a > lot. > > > Could > > > > > > you sketch out to me the path which would resulting in destroying the > > > entry > > > > > > while there are HC::Ts that were once associated with it? > > > > > > > > > > > > If there is a failure then waiting_for_read_ transactions' callbacks > are > > > invoked > > > > > > with the error, idle transactions have a state set which will be > checked > > > in > > > > > > their future Read calls and Writers is then empty. Writers will then > > > invoke > > > > > > cache_->DoneWritingToEntry with success as false leading to > > > ProcessEntryFailure > > > > > > being called which will destroy the entry and restart any > > > add_to_entry_queue > > > > > > transactions. > > > > > > > > > > Makes sense. Ok, I'm slightly in favor at the architectural level of > > > dooming the entry but not destroying it while there are still associated > > > transactions--the AE is the coordination point, so it makes sense to me for > it > > > to hold the error state. Having said that, as I say above I don't think > it's > > > worth a lot of code changes, so feel free to do that or not as you prefer. > > > > > > > > Changed the logic to keep this state in Writers itself as that seems > simpler. > > > > > > Actually while working on the follow up CL, a use case came up because of > which > > > the earlier design would be preferable which sets the state in the > individual > > > transactions and not in the Writers class. So, let's say there is a cache > write > > > failure. Now we want a fail state for future Reads for all idle transactions > but > > > not for the transaction that actually had the cache write failure since it > can > > > continue to read from the network through writers. So making that change in > the > > > follow up CL that integrates this class with HC::T and HC. > > > > Accepted if that's how you decide that's the best way to handle it, but in the > case you describe, the cache write is happening on behalf of the active > transaction. So you could set the fail state on the writers class for future > reads, while returning success for the current transaction. I think that's > conceptually clean--all future reads bounce, but Read() has already been called > for the transaction that you want to return successfully. > > We actually want future reads on the active_transaction_ to go through as well. > Since network_read_only_ will be set to true, these future reads will only read > from the network. Ah, makes sense. Drat. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:169: ResetStateForEmptyWriters(); On 2017/06/08 17:55:56, shivanisha wrote: > On 2017/06/08 at 17:10:00, Randy Smith (Not in Mondays) wrote: > > On 2017/06/07 15:19:18, shivanisha wrote: > > > On 2017/06/06 at 12:54:57, Randy Smith (Not in Mondays) wrote: > > > > Shouldn't this already have happened if all_writers_ is empty? Seems like > it > > > should be a DCHECK(), not a call. > > > > > > This basically resets some member variables and should be invoked when the > all > > > the transactions have been removed which is what is happening in the > statements > > > above. > > > > This makes the interface contract for ProcessWaitingForReadTransaction() a bit > messy, in that it's now implicitly the responsibility of the caller of > ProcessWaitingForReadTransactions() to call ResetStateForEmptyWriters(). Would > you be willing to move the state reset to the end of PWFRT() outside the loop? > That's an extra check in the failure case, but beyond that matches the > performance of the current implementation, and leaves the responsibilities > cleaner. > > ResetStateForEmptyWriters() is not really related to > ProcessWaitingForReadTransaction(). ProcessWaitingForReadTransaction takes care > of processing the IO callbacks of waiting transactions and > ResetStateForEmptyWriters is responsible for resetting state in other cases as > well and not just when waiting_for_read is processed. So I see them as separate > responsibilities and here ProcessFailure() takes care of both responsibilities. TL;DR: I want to make on more try at explaining my perspective, then I'll accept whatever decision you make. The flip side of this argument: You want to call ResetStateForEmptyWriters() whenever all_writers_ transitions to empty(), and you never want it to be called in any other case. You could imagine (not recommending or requesting this!) creating a wrapper class for all_writers_ and it's related state, and having it automatically track the state of all_writers_ and execute the logic in ResetStateForEmptyWriters() whenever all_writers_ transitions to empty. Given that (IMO, and I suspect yours) the right choice in terms of code clarity is not to do the wrapping, I'd argue that the next best thing is to keep the reset as near as possible to the code clearing all_writers_. This has two advantages: Reviewers can more easily validate the invariant that ResetStateForEmptyWriters() if-and-only-if all_writers_ has transitioned to empty, and future modifiers if this code, if they copy a section that includes modifying all_writers_, have a higher chance of copying the call to ResetStateForEmptyWriters(). Pull up a level to address your conceptual argument: I agree that ResetStateForEmptyWriters() isn't related to ProcessWaitingForReadTransaction(), but it's related to the lower-level concept that ProcessWaitingForReadTransaction() also affects, which is the state of all_writers_. Given that, I'd put it near the state change for all_writers_. And, having said all that, your call--this isn't a big deal. I just wanted to try and make clear why my preference was the other direction. https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.cc:279: int current_size = entry_->disk_entry->GetDataSize(kResponseContentIndex); On 2017/06/08 17:55:56, shivanisha wrote: > On 2017/06/08 at 17:10:01, Randy Smith (Not in Mondays) wrote: > > Why is this accurate as the offset to which to write in the cache in a partial > case? Couldn't a partial read be anywhere in the entry? > > > WriteToEntry => partial->CacheWrite which does not take the offset. Ah, right. I'd afraid I'd call that a confusing interface. Willing to document (in the headers or before the definition; for private functions I don't think there's a lot of difference) that WriteToEntry ignores the offset argument int in the partial case? Alternatively, you could just hoist the code for WriteToEntry() up into DoCacheWriteData(); it doesn't look to be called anywhere else. https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:20: class HttpCache::WritersTest { On 2017/06/12 18:30:33, jkarlin wrote: > Since every test uses this class, it seems like this ought to be a test fixture > class. +1
PTAL, Thanks! Addressed all feedback and included new tests. https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/260001/net/http/http_cache_wr... net/http/http_cache_writers.cc:169: ResetStateForEmptyWriters(); On 2017/06/13 at 17:45:05, Randy Smith (Not in Mondays) wrote: > On 2017/06/08 17:55:56, shivanisha wrote: > > On 2017/06/08 at 17:10:00, Randy Smith (Not in Mondays) wrote: > > > On 2017/06/07 15:19:18, shivanisha wrote: > > > > On 2017/06/06 at 12:54:57, Randy Smith (Not in Mondays) wrote: > > > > > Shouldn't this already have happened if all_writers_ is empty? Seems like > > it > > > > should be a DCHECK(), not a call. > > > > > > > > This basically resets some member variables and should be invoked when the > > all > > > > the transactions have been removed which is what is happening in the > > statements > > > > above. > > > > > > This makes the interface contract for ProcessWaitingForReadTransaction() a bit > > messy, in that it's now implicitly the responsibility of the caller of > > ProcessWaitingForReadTransactions() to call ResetStateForEmptyWriters(). Would > > you be willing to move the state reset to the end of PWFRT() outside the loop? > > That's an extra check in the failure case, but beyond that matches the > > performance of the current implementation, and leaves the responsibilities > > cleaner. > > > > ResetStateForEmptyWriters() is not really related to > > ProcessWaitingForReadTransaction(). ProcessWaitingForReadTransaction takes care > > of processing the IO callbacks of waiting transactions and > > ResetStateForEmptyWriters is responsible for resetting state in other cases as > > well and not just when waiting_for_read is processed. So I see them as separate > > responsibilities and here ProcessFailure() takes care of both responsibilities. > > TL;DR: I want to make on more try at explaining my perspective, then I'll accept whatever decision you make. > > The flip side of this argument: You want to call ResetStateForEmptyWriters() whenever all_writers_ transitions to empty(), and you never want it to be called in any other case. You could imagine (not recommending or requesting this!) creating a wrapper class for all_writers_ and it's related state, and having it automatically track the state of all_writers_ and execute the logic in ResetStateForEmptyWriters() whenever all_writers_ transitions to empty. > > Given that (IMO, and I suspect yours) the right choice in terms of code clarity is not to do the wrapping, I'd argue that the next best thing is to keep the reset as near as possible to the code clearing all_writers_. This has two advantages: Reviewers can more easily validate the invariant that ResetStateForEmptyWriters() if-and-only-if all_writers_ has transitioned to empty, and future modifiers if this code, if they copy a section that includes modifying all_writers_, have a higher chance of copying the call to ResetStateForEmptyWriters(). > > Pull up a level to address your conceptual argument: I agree that ResetStateForEmptyWriters() isn't related to ProcessWaitingForReadTransaction(), but it's related to the lower-level concept that ProcessWaitingForReadTransaction() also affects, which is the state of all_writers_. Given that, I'd put it near the state change for all_writers_. > > And, having said all that, your call--this isn't a big deal. I just wanted to try and make clear why my preference was the other direction. I understand the concern here that it's not obvious when ResetState.. should be invoked . But there is a change in the latest patch which is that instead of asserting that all_writers is empty , it checks if all_writers_ is empty and calls ResetState.. if it is. This is because in cache write failure case we do not remove the active_transaction_ from all_writers_ but in network failure case, we do. So conceptually ResetState... is called in all the ending scenarios where all_writers_ "may" become empty: RemoveTransaction() OnDataReceived() // success ProcessFailure() // failure Also commenting this requirement in headers where all_writers_ is declared. https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_tr... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_tr... net/http/http_cache_transaction.h:170: RequestPriority priority() { return priority_; } On 2017/06/12 at 18:30:32, jkarlin wrote: > this should be a const function done https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.cc:83: DCHECK_EQ(all_writers_.size(), (size_t)1); On 2017/06/12 at 18:30:32, jkarlin wrote: > DCHECK_EQ(1u, all_writers_.size()); > > DCHECK's crash message is written such that the expected value is the first argument. done https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.cc:92: DCHECK(network_transaction_); On 2017/06/12 at 18:30:32, jkarlin wrote: > I'd prefer we moved this to the top of the function: > > DCHECK(network_transaction_ || network_transaction); done https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.cc:99: if (!transaction) On 2017/06/12 at 18:30:32, jkarlin wrote: > Can we instead DCHECK(transaction)? Explicitly handling nullptr for the following scenario: active_transaction_ is destroyed mid-read, when buffer is written to cache DoneWritingToEntry is called with nullptr as the transaction and that invokes RemoveTransaction(nullptr). https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.cc:109: PriorityChanged(); On 2017/06/12 at 18:30:32, jkarlin wrote: > Perhaps rename to UpdatePriority since the priority may or may not have changed? done https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.cc:120: waiting_for_read_.erase(waiting_it); On 2017/06/12 at 18:30:32, jkarlin wrote: > Hmm, is the iterator still safe to increment after calling erase? Do we have a test covering the case of RemoveTransaction when there are multiple transactions? we do not increment the iterator after erase due to the following return. https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.cc:146: // Should be invoked after |waiting_for_read_| transactions and On 2017/06/12 at 18:30:32, jkarlin wrote: > extra space between transactions and and done https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.cc:151: entry_->readers.insert(idle_writer); On 2017/06/12 at 18:30:32, jkarlin wrote: > I think at this point we should treat ActiveEntry as a class instead of a struct. Directly injecting into readers here means that there are now two files mutating ActiveEntries. For now, can you add a function to HttpCache for reader insertion? done https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.cc:279: int current_size = entry_->disk_entry->GetDataSize(kResponseContentIndex); On 2017/06/13 at 17:45:05, Randy Smith (Not in Mondays) wrote: > On 2017/06/08 17:55:56, shivanisha wrote: > > On 2017/06/08 at 17:10:01, Randy Smith (Not in Mondays) wrote: > > > Why is this accurate as the offset to which to write in the cache in a partial > > case? Couldn't a partial read be anywhere in the entry? > > > > > > WriteToEntry => partial->CacheWrite which does not take the offset. > > Ah, right. I'd afraid I'd call that a confusing interface. Willing to document (in the headers or before the definition; for private functions I don't think there's a lot of difference) that WriteToEntry ignores the offset argument int in the partial case? Alternatively, you could just hoist the code for WriteToEntry() up into DoCacheWriteData(); it doesn't look to be called anywhere else. Moved to calling site. https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.h:7: #include <list> On 2017/06/12 at 18:30:32, jkarlin wrote: > newline above Added https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.h:9: #include "base/memory/weak_ptr.h" On 2017/06/12 at 18:30:32, jkarlin wrote: > newline above added https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.h:49: // Adds an HttpCache::Transaction to Writers and if its the first transaction On 2017/06/12 at 18:30:32, jkarlin wrote: > s/its/it's/ done https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.h:149: On 2017/06/12 at 18:30:32, jkarlin wrote: > Remove newline? Comment applies to all 3 following functions. https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.h:226: #endif // NET_HTTP_HTTP_CACHE_WRITERS_H_ On 2017/06/12 at 18:30:32, jkarlin wrote: > newline above Added https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:20: class HttpCache::WritersTest { On 2017/06/13 at 17:45:05, Randy Smith (Not in Mondays) wrote: > On 2017/06/12 18:30:33, jkarlin wrote: > > Since every test uses this class, it seems like this ought to be a test fixture > > class. > > +1 This class is mostly a collection of helper functions and does not follow any Setup and TearDown . Each test requires a different collection of helper functions from this class. https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:215: } On 2017/06/12 at 18:30:33, jkarlin wrote: > I'd like to see tests for all of the public API in this file, with lots of tests for behavior of multiple writers with writers/readers coming and going. Added tests for all public APIs, multiple Reads , deletion of active/waiting/idle transactions while reading, cache write failure and network read failure cases.
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Initial comments on tests. https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:20: class HttpCache::WritersTest { On 2017/06/14 02:33:17, shivanisha wrote: > On 2017/06/13 at 17:45:05, Randy Smith (Not in Mondays) wrote: > > On 2017/06/12 18:30:33, jkarlin wrote: > > > Since every test uses this class, it seems like this ought to be a test > fixture > > > class. > > > > +1 > > This class is mostly a collection of helper functions and does not follow any > Setup and TearDown . Each test requires a different collection of helper > functions from this class. Sure. But if you put them in a text fixture with a null setup and teardown (I don't think you need to declare the functions if you aren't using them) they're accessible to all tests in that fixture without your having to declare them in each test. I think of this as what a text fixture is for (collecting a set of helper functions in one place that'll be easily available to a set of tests). https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:34: writers_ = base::MakeUnique<Writers>(cache, entry); This makes me a bit uncomfortable, in that HC::W is intended to be a value in ActiveEntry. I understand that switchover will be in a future CL, but could you put in a TODO about shifting this over to being hosted in the HC::AE? https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:37: void CreateNetworkTransaction(std::unique_ptr<HttpTransaction>* transaction) { nit, suggestion: Why not just return a unique_ptr<HttpTransaction>? https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:70: void AddTransactionToExistingWriters() { Hmmm. This looks to be creating a transaction as much as the previous function, so the nomenclature doesn't seem consistent. Could you making it so? (Adding "Create" here or removing it above?) https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:115: rv = writers_->Read(bufs[i].get(), 256, callbacks[i].callback(), Worthwhile having a test with multiple writers with different sized buffers? (Long term, after integration, we'll want to test that situation with them reading different amounts, but I presume what'll happen then is the short reads will hit in cache when they come back, which I think is being implemented by integration, not by this CL.) https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:116: (Transaction*)transaction.get()); This drops rv on the floor, which seems like a mistake. Shouldn't we DCHECK or EXPECT it? Or make sure it's all the same result, so that after this loop we can test rv and have it apply to all of the calls? https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:120: if (rv == ERR_IO_PENDING) { This treats as special the rv from the last Read() call. Why? https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:122: rv = callback.WaitForResult(); What if one of the previous calls completed synchronously? (If you're counting on that not happening because you know the implementation, at least put a comment in to that effect.) https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:125: if (rv > 0) { Same set of concerns as above: If the rvs are different, this won't catch that. https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:146: // Multiple transactions should be able to read. Suggest helper function that encapsulates the handling of the multiple transactions (above, here, and below). https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:375: Transaction* transaction = (Transaction*)(transactions_.begin()->get()); nit, suggestion: DCHECK there's a transaction there? https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:492: // mid-read. Any waiting for read transaction should be able to get the read nit: Any transactions waiting ...
https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers.h:149: On 2017/06/14 02:33:17, shivanisha wrote: > On 2017/06/12 at 18:30:32, jkarlin wrote: > > Remove newline? > > Comment applies to all 3 following functions. Right, for group comments I typically see the comment directly above the first function, and it applies to all functions below until empty line. https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:34: writers_ = base::MakeUnique<Writers>(cache, entry); On 2017/06/14 19:49:18, Randy Smith (Not in Mondays) wrote: > This makes me a bit uncomfortable, in that HC::W is intended to be a value in > ActiveEntry. I understand that switchover will be in a future CL, but could you > put in a TODO about shifting this over to being hosted in the HC::AE? I'm not sure I'm following. In my mind this file should be explicitly testing the Writers interface. So it should create a Writers object directly and test the public API with as few dependencies (ActiveEntry/HttpCache) as possible. For integration tests, we'd use the http_cache_unittests. WDYT?
https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:34: writers_ = base::MakeUnique<Writers>(cache, entry); On 2017/06/14 20:03:14, jkarlin wrote: > On 2017/06/14 19:49:18, Randy Smith (Not in Mondays) wrote: > > This makes me a bit uncomfortable, in that HC::W is intended to be a value in > > ActiveEntry. I understand that switchover will be in a future CL, but could > you > > put in a TODO about shifting this over to being hosted in the HC::AE? > > I'm not sure I'm following. In my mind this file should be explicitly testing > the Writers interface. So it should create a Writers object directly and test > the public API with as few dependencies (ActiveEntry/HttpCache) as possible. For > integration tests, we'd use the http_cache_unittests. WDYT? I completely agree in the abstract. But to me, that abstract is for cases where you can separate out the object from linkages with other objects. The above code creates an ActiveEntry in the cache, then creates a writers_ object outside the cache. I'm concerned about that mismatch; I think it wouldn't take major changes in implementation for that to break things. If writers_ could exist without reference to a cache/AE, test it in isolation. But if it's gotta be initialized with actual instances of those objects (not even mocks), I'd put it in HC::AE as well. (I could be argued out of this; I'm not going to go to the wall. But the above is why I made the recommendation I did.)
https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:34: writers_ = base::MakeUnique<Writers>(cache, entry); > If writers_ could exist > without reference to a cache/AE, test it in isolation. I'd like to try this. It looks like HttpCache* is ignored by the Writers constructor. The ActiveEntry is used to get at the disk entry for all but one usage, so perhaps we could pass a disk entry to the constructor instead of an ActiveEntry if we could find a good way to avoid the call to entry_->AddTransactionToReaders(), which seems out of place in Writers anyway.
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Patchset #15 (id:420001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, Thanks! Feedback addressed. https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:20: class HttpCache::WritersTest { On 2017/06/14 at 19:49:17, Randy Smith (Not in Mondays) wrote: > On 2017/06/14 02:33:17, shivanisha wrote: > > On 2017/06/13 at 17:45:05, Randy Smith (Not in Mondays) wrote: > > > On 2017/06/12 18:30:33, jkarlin wrote: > > > > Since every test uses this class, it seems like this ought to be a test > > fixture > > > > class. > > > > > > +1 > > > > This class is mostly a collection of helper functions and does not follow any > > Setup and TearDown . Each test requires a different collection of helper > > functions from this class. > > Sure. But if you put them in a text fixture with a null setup and teardown (I don't think you need to declare the functions if you aren't using them) they're accessible to all tests in that fixture without your having to declare them in each test. I think of this as what a text fixture is for (collecting a set of helper functions in one place that'll be easily available to a set of tests). Since the class needs to access a private nested class of HttpCache which is HttpCache::Writers, it needs to itself be a nested class of HttpCache? And then if I create as a fixture class the test with macro TEST_F does not take HttpCache::WritersTest or WritersTest. Am I missing something here? https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:37: void CreateNetworkTransaction(std::unique_ptr<HttpTransaction>* transaction) { On 2017/06/14 at 19:49:18, Randy Smith (Not in Mondays) wrote: > nit, suggestion: Why not just return a unique_ptr<HttpTransaction>? done https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:70: void AddTransactionToExistingWriters() { On 2017/06/14 at 19:49:18, Randy Smith (Not in Mondays) wrote: > Hmmm. This looks to be creating a transaction as much as the previous function, so the nomenclature doesn't seem consistent. Could you making it so? (Adding "Create" here or removing it above?) Since creation is for writers in the nomenclature and not for a transaction, renaming the above function to CreateWritersAddTransaction https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:115: rv = writers_->Read(bufs[i].get(), 256, callbacks[i].callback(), On 2017/06/14 at 19:49:17, Randy Smith (Not in Mondays) wrote: > Worthwhile having a test with multiple writers with different sized buffers? > > (Long term, after integration, we'll want to test that situation with them reading different amounts, but I presume what'll happen then is the short reads will hit in cache when they come back, which I think is being implemented by integration, not by this CL.) That's right, since the shorter buffer transaction will need to read the cache, that cannot be tested in this CL. https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:116: (Transaction*)transaction.get()); On 2017/06/14 at 19:49:17, Randy Smith (Not in Mondays) wrote: > This drops rv on the floor, which seems like a mistake. Shouldn't we DCHECK or EXPECT it? Or make sure it's all the same result, so that after this loop we can test rv and have it apply to all of the calls? Done https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:120: if (rv == ERR_IO_PENDING) { On 2017/06/14 at 19:49:18, Randy Smith (Not in Mondays) wrote: > This treats as special the rv from the last Read() call. Why? Asserted that all rv is ERR_IO_PENDING for all calls. And also checking that the rv returned from WaitForResult is same for all transactions. https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:122: rv = callback.WaitForResult(); On 2017/06/14 at 19:49:18, Randy Smith (Not in Mondays) wrote: > What if one of the previous calls completed synchronously? (If you're counting on that not happening because you know the implementation, at least put a comment in to that effect.) done. Default behavior is asynchronous. https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:125: if (rv > 0) { On 2017/06/14 at 19:49:17, Randy Smith (Not in Mondays) wrote: > Same set of concerns as above: If the rvs are different, this won't catch that. Addressed in all functions. https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:146: // Multiple transactions should be able to read. On 2017/06/14 at 19:49:17, Randy Smith (Not in Mondays) wrote: > Suggest helper function that encapsulates the handling of the multiple transactions (above, here, and below). They all differ a bit inside the loop based on the scenario being tested. https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:375: Transaction* transaction = (Transaction*)(transactions_.begin()->get()); On 2017/06/14 at 19:49:18, Randy Smith (Not in Mondays) wrote: > nit, suggestion: DCHECK there's a transaction there? done here and below https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:492: // mid-read. Any waiting for read transaction should be able to get the read On 2017/06/14 at 19:49:18, Randy Smith (Not in Mondays) wrote: > nit: Any transactions waiting ... done
https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:34: writers_ = base::MakeUnique<Writers>(cache, entry); On 2017/06/14 at 20:28:37, jkarlin wrote: > > If writers_ could exist > > without reference to a cache/AE, test it in isolation. > > I'd like to try this. It looks like HttpCache* is ignored by the Writers constructor. The ActiveEntry is used to get at the disk entry for all but one usage, so perhaps we could pass a disk entry to the constructor instead of an ActiveEntry if we could find a good way to avoid the call to entry_->AddTransactionToReaders(), which seems out of place in Writers anyway. Changed the implementation of MoveIdleWritersToReaders to removeAllIdleWriters() which returns a set of transactions and HttpCache can then add them to readers. Passed disk entry instead of active entry in the constructor.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Next round of comments on tests. Converging :-}. https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:20: class HttpCache::WritersTest { On 2017/06/15 20:17:45, shivanisha wrote: > On 2017/06/14 at 19:49:17, Randy Smith (Not in Mondays) wrote: > > On 2017/06/14 02:33:17, shivanisha wrote: > > > On 2017/06/13 at 17:45:05, Randy Smith (Not in Mondays) wrote: > > > > On 2017/06/12 18:30:33, jkarlin wrote: > > > > > Since every test uses this class, it seems like this ought to be a test > > > fixture > > > > > class. > > > > > > > > +1 > > > > > > This class is mostly a collection of helper functions and does not follow > any > > > Setup and TearDown . Each test requires a different collection of helper > > > functions from this class. > > > > Sure. But if you put them in a text fixture with a null setup and teardown (I > don't think you need to declare the functions if you aren't using them) they're > accessible to all tests in that fixture without your having to declare them in > each test. I think of this as what a text fixture is for (collecting a set of > helper functions in one place that'll be easily available to a set of tests). > > Since the class needs to access a private nested class of HttpCache which is > HttpCache::Writers, it needs to itself be a nested class of HttpCache? Fair, though there are several ways around that. You could certainly make it a non-private class for testing, though that's got similar problems as to this approach. I would also think that creating a test fixture and making that a friend of both HttpCache and HttpCache::Writers would work, and would be in line with other existing test structuring. > And then > if I create as a fixture class the test with macro TEST_F does not take > HttpCache::WritersTest or WritersTest. Am I missing something here? I wouldn't expect it to anything with a :: in it--it would need to be (in the test file) a top level class. (I'm not dead set on the structures I'm pushing, but I do think that this is unusual, and the structures I'm pushing more in line with existing Chromium practice.) https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:34: writers_ = base::MakeUnique<Writers>(cache, entry); On 2017/06/15 20:46:26, shivanisha wrote: > On 2017/06/14 at 20:28:37, jkarlin wrote: > > > If writers_ could exist > > > without reference to a cache/AE, test it in isolation. > > > > I'd like to try this. It looks like HttpCache* is ignored by the Writers > constructor. The ActiveEntry is used to get at the disk entry for all but one > usage, so perhaps we could pass a disk entry to the constructor instead of an > ActiveEntry if we could find a good way to avoid the call to > entry_->AddTransactionToReaders(), which seems out of place in Writers anyway. > > Changed the implementation of MoveIdleWritersToReaders to removeAllIdleWriters() > which returns a set of transactions and HttpCache can then add them to readers. > Passed disk entry instead of active entry in the constructor. > FWIW, This doesn't address my concerns in that there's still a partial entanglement of objects without it being the entanglement that is actually used in production code. If at some point in the future code makes some assumption about being able to start at a Writers object and find that object in the referred cache (even though that code no longer has the shortcut of a pointer to the ActiveEntry), this will break. I do agree with Josh that in the ideal case, unit tests for a class should test that class in isolation. But the way that that would lead for me in this case is mocking out HttpCache and DiskEntry, not trying to minimize the possible pathways between production objects that would lead to a contradiction. The alternative (which I don't have objections to, it's just not the platonic ideal of testing) is to accept that certain classes always expect to work with each other, and test them in the relationship they have in production code. I'm happy to re-raise this when we get to the integration CL; I'm not asking for any current changes. I'm just calling out that this doesn't improve things much along the axis that concerned me. https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:115: rv = writers_->Read(bufs[i].get(), 256, callbacks[i].callback(), On 2017/06/15 20:17:46, shivanisha wrote: > On 2017/06/14 at 19:49:17, Randy Smith (Not in Mondays) wrote: > > Worthwhile having a test with multiple writers with different sized buffers? > > > > (Long term, after integration, we'll want to test that situation with them > reading different amounts, but I presume what'll happen then is the short reads > will hit in cache when they come back, which I think is being implemented by > integration, not by this CL.) > > That's right, since the shorter buffer transaction will need to read the cache, > that cannot be tested in this CL. Sorry, why? I understand why doing multiple reads on a single transaction would need to read the cache, but why can't there be multiple transactions with different buffer sizes? https://codereview.chromium.org/2886483002/diff/440001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/440001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:84: Transaction* transaction = (Transaction*)(transactions_.begin()->get()); DCHECK that there's at least one transaction in transactions_? https://codereview.chromium.org/2886483002/diff/440001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:134: for (auto& result : results) { suggestion-but-I'd-prefer-it: Using *both* the auto syntax and incrementing an index seems vulnerable to getting out of sync. Would you be willing to do for (i =0; i < results.size(); i++) { ... results[i].append(...}? https://codereview.chromium.org/2886483002/diff/440001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:186: } else if (rv <= 0) { suggestion: If you eliminate this conditional and return rv instead of OK in the last line, doesn't that have the same effect? https://codereview.chromium.org/2886483002/diff/440001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:232: for (auto& result : results) { Same request as above--if the loop is going to be using an index variable and incrementing it each time around the loop, I'd rather that be the primary variable and result looked up via results[i]. (Applies anywhere else in the file this pattern occurs.) https://codereview.chromium.org/2886483002/diff/440001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:241: return rv; Same suggestion as above; delete this clause and return rv at end of function? (Applies anywhere else in the file this pattern occurs.) https://codereview.chromium.org/2886483002/diff/440001/net/http/http_transact... File net/http/http_transaction_test_util.cc (right): https://codereview.chromium.org/2886483002/diff/440001/net/http/http_transact... net/http/http_transaction_test_util.cc:306: if (!num) { I find it surprising that if both read_error_ and the read_handler_ is set, read_error_ wins. I think that means that the handler associated with any mock transactions added with AddMockTransaction are ignored if read_error_ is set, but not the other attributes added with AddMockTransaction. If this is the behavior you want, could you work out in detail what that behavior is going to be and document it in the header file? If not, could you make read_handler_ being set override read_error_ being set (and document that behavior)?
https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:34: writers_ = base::MakeUnique<Writers>(cache, entry); On 2017/06/22 21:45:05, Randy Smith (Not in Mondays) wrote: > On 2017/06/15 20:46:26, shivanisha wrote: > > On 2017/06/14 at 20:28:37, jkarlin wrote: > > > > If writers_ could exist > > > > without reference to a cache/AE, test it in isolation. > > > > > > I'd like to try this. It looks like HttpCache* is ignored by the Writers > > constructor. The ActiveEntry is used to get at the disk entry for all but one > > usage, so perhaps we could pass a disk entry to the constructor instead of an > > ActiveEntry if we could find a good way to avoid the call to > > entry_->AddTransactionToReaders(), which seems out of place in Writers anyway. > > > > Changed the implementation of MoveIdleWritersToReaders to > removeAllIdleWriters() > > which returns a set of transactions and HttpCache can then add them to > readers. > > Passed disk entry instead of active entry in the constructor. > > > > FWIW, This doesn't address my concerns in that there's still a partial > entanglement of objects without it being the entanglement that is actually used > in production code. If at some point in the future code makes some assumption > about being able to start at a Writers object and find that object in the > referred cache (even though that code no longer has the shortcut of a pointer to > the ActiveEntry), this will break. > > I do agree with Josh that in the ideal case, unit tests for a class should test > that class in isolation. But the way that that would lead for me in this case > is mocking out HttpCache and DiskEntry, not trying to minimize the possible > pathways between production objects that would lead to a contradiction. The > alternative (which I don't have objections to, it's just not the platonic ideal > of testing) is to accept that certain classes always expect to work with each > other, and test them in the relationship they have in production code. > > I'm happy to re-raise this when we get to the integration CL; I'm not asking for > any current changes. I'm just calling out that this doesn't improve things much > along the axis that concerned me. So the HttpCache argument isn't used, we can (and should) remove that. In which case the only thing left to mock is the entry, and there exists a MockDiskEntry class for that, which I agree we should use.
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Patchset #17 (id:480001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, Thanks! Last 2 patches rebase and address test feedback respectively. https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:34: writers_ = base::MakeUnique<Writers>(cache, entry); On 2017/06/26 at 16:58:42, jkarlin_slow wrote: > On 2017/06/22 21:45:05, Randy Smith (Not in Mondays) wrote: > > On 2017/06/15 20:46:26, shivanisha wrote: > > > On 2017/06/14 at 20:28:37, jkarlin wrote: > > > > > If writers_ could exist > > > > > without reference to a cache/AE, test it in isolation. > > > > > > > > I'd like to try this. It looks like HttpCache* is ignored by the Writers > > > constructor. The ActiveEntry is used to get at the disk entry for all but one > > > usage, so perhaps we could pass a disk entry to the constructor instead of an > > > ActiveEntry if we could find a good way to avoid the call to > > > entry_->AddTransactionToReaders(), which seems out of place in Writers anyway. > > > > > > Changed the implementation of MoveIdleWritersToReaders to > > removeAllIdleWriters() > > > which returns a set of transactions and HttpCache can then add them to > > readers. > > > Passed disk entry instead of active entry in the constructor. > > > > > > > FWIW, This doesn't address my concerns in that there's still a partial > > entanglement of objects without it being the entanglement that is actually used > > in production code. If at some point in the future code makes some assumption > > about being able to start at a Writers object and find that object in the > > referred cache (even though that code no longer has the shortcut of a pointer to > > the ActiveEntry), this will break. > > > > I do agree with Josh that in the ideal case, unit tests for a class should test > > that class in isolation. But the way that that would lead for me in this case > > is mocking out HttpCache and DiskEntry, not trying to minimize the possible > > pathways between production objects that would lead to a contradiction. The > > alternative (which I don't have objections to, it's just not the platonic ideal > > of testing) is to accept that certain classes always expect to work with each > > other, and test them in the relationship they have in production code. > > > > I'm happy to re-raise this when we get to the integration CL; I'm not asking for > > any current changes. I'm just calling out that this doesn't improve things much > > along the axis that concerned me. > > So the HttpCache argument isn't used, we can (and should) remove that. In which case the only thing left to mock is the entry, and there exists a MockDiskEntry class for that, which I agree we should use. Removing cache argument from this CL (will be added back in the follow up CL). Also using MockDiskEntry instead of an actual disk_cache::entry for creating Writers. https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:115: rv = writers_->Read(bufs[i].get(), 256, callbacks[i].callback(), On 2017/06/22 at 21:45:05, Randy Smith (Not in Mondays) wrote: > On 2017/06/15 20:17:46, shivanisha wrote: > > On 2017/06/14 at 19:49:17, Randy Smith (Not in Mondays) wrote: > > > Worthwhile having a test with multiple writers with different sized buffers? > > > > > > (Long term, after integration, we'll want to test that situation with them > > reading different amounts, but I presume what'll happen then is the short reads > > will hit in cache when they come back, which I think is being implemented by > > integration, not by this CL.) > > > > That's right, since the shorter buffer transaction will need to read the cache, > > that cannot be tested in this CL. > > Sorry, why? I understand why doing multiple reads on a single transaction would need to read the cache, but why can't there be multiple transactions with different buffer sizes? If there are two transactions reading simultaneously and the second has a smaller buffer than the first, the second should be reading the delta data from the cache but cache reading is part of the HC::T state machine and not writers and should thus be tested in integration CL. https://codereview.chromium.org/2886483002/diff/440001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/440001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:84: Transaction* transaction = (Transaction*)(transactions_.begin()->get()); On 2017/06/22 at 21:45:05, Randy Smith (Not in Mondays) wrote: > DCHECK that there's at least one transaction in transactions_? done https://codereview.chromium.org/2886483002/diff/440001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:134: for (auto& result : results) { On 2017/06/22 at 21:45:05, Randy Smith (Not in Mondays) wrote: > suggestion-but-I'd-prefer-it: Using *both* the auto syntax and incrementing an index seems vulnerable to getting out of sync. Would you be willing to do for (i =0; i < results.size(); i++) { ... results[i].append(...}? done https://codereview.chromium.org/2886483002/diff/440001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:186: } else if (rv <= 0) { On 2017/06/22 at 21:45:05, Randy Smith (Not in Mondays) wrote: > suggestion: If you eliminate this conditional and return rv instead of OK in the last line, doesn't that have the same effect? done https://codereview.chromium.org/2886483002/diff/440001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:232: for (auto& result : results) { On 2017/06/22 at 21:45:05, Randy Smith (Not in Mondays) wrote: > Same request as above--if the loop is going to be using an index variable and incrementing it each time around the loop, I'd rather that be the primary variable and result looked up via results[i]. > > (Applies anywhere else in the file this pattern occurs.) done https://codereview.chromium.org/2886483002/diff/440001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:241: return rv; On 2017/06/22 at 21:45:05, Randy Smith (Not in Mondays) wrote: > Same suggestion as above; delete this clause and return rv at end of function? > > (Applies anywhere else in the file this pattern occurs.) done https://codereview.chromium.org/2886483002/diff/440001/net/http/http_transact... File net/http/http_transaction_test_util.cc (right): https://codereview.chromium.org/2886483002/diff/440001/net/http/http_transact... net/http/http_transaction_test_util.cc:306: if (!num) { On 2017/06/22 at 21:45:05, Randy Smith (Not in Mondays) wrote: > I find it surprising that if both read_error_ and the read_handler_ is set, read_error_ wins. I think that means that the handler associated with any mock transactions added with AddMockTransaction are ignored if read_error_ is set, but not the other attributes added with AddMockTransaction. If this is the behavior you want, could you work out in detail what that behavior is going to be and document it in the header file? If not, could you make read_handler_ being set override read_error_ being set (and document that behavior)? Yes, the intention is that if error is set it will override all Read logic and simply return the error code (synchronously/asynchronously based on test mode). Documented the same.
https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:20: class HttpCache::WritersTest { On 2017/06/22 at 21:45:05, Randy Smith (Not in Mondays) wrote: > On 2017/06/15 20:17:45, shivanisha wrote: > > On 2017/06/14 at 19:49:17, Randy Smith (Not in Mondays) wrote: > > > On 2017/06/14 02:33:17, shivanisha wrote: > > > > On 2017/06/13 at 17:45:05, Randy Smith (Not in Mondays) wrote: > > > > > On 2017/06/12 18:30:33, jkarlin wrote: > > > > > > Since every test uses this class, it seems like this ought to be a test > > > > fixture > > > > > > class. > > > > > > > > > > +1 > > > > > > > > This class is mostly a collection of helper functions and does not follow > > any > > > > Setup and TearDown . Each test requires a different collection of helper > > > > functions from this class. > > > > > > Sure. But if you put them in a text fixture with a null setup and teardown (I > > don't think you need to declare the functions if you aren't using them) they're > > accessible to all tests in that fixture without your having to declare them in > > each test. I think of this as what a text fixture is for (collecting a set of > > helper functions in one place that'll be easily available to a set of tests). > > > > Since the class needs to access a private nested class of HttpCache which is > > HttpCache::Writers, it needs to itself be a nested class of HttpCache? > > Fair, though there are several ways around that. You could certainly make it a non-private class for testing, though that's got similar problems as to this approach. I would also think that creating a test fixture and making that a friend of both HttpCache and HttpCache::Writers would work, and would be in line with other existing test structuring. > > > And then > > if I create as a fixture class the test with macro TEST_F does not take > > HttpCache::WritersTest or WritersTest. Am I missing something here? > > I wouldn't expect it to anything with a :: in it--it would need to be (in the test file) a top level class. > > (I'm not dead set on the structures I'm pushing, but I do think that this is unusual, and the structures I'm pushing more in line with existing Chromium practice.) I will address this tomorrow but didn't want to delay uploading the other changes on this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Converted the test class to a test fixture class. https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/340001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:20: class HttpCache::WritersTest { On 2017/06/26 at 20:47:02, shivanisha wrote: > On 2017/06/22 at 21:45:05, Randy Smith (Not in Mondays) wrote: > > On 2017/06/15 20:17:45, shivanisha wrote: > > > On 2017/06/14 at 19:49:17, Randy Smith (Not in Mondays) wrote: > > > > On 2017/06/14 02:33:17, shivanisha wrote: > > > > > On 2017/06/13 at 17:45:05, Randy Smith (Not in Mondays) wrote: > > > > > > On 2017/06/12 18:30:33, jkarlin wrote: > > > > > > > Since every test uses this class, it seems like this ought to be a test > > > > > fixture > > > > > > > class. > > > > > > > > > > > > +1 > > > > > > > > > > This class is mostly a collection of helper functions and does not follow > > > any > > > > > Setup and TearDown . Each test requires a different collection of helper > > > > > functions from this class. > > > > > > > > Sure. But if you put them in a text fixture with a null setup and teardown (I > > > don't think you need to declare the functions if you aren't using them) they're > > > accessible to all tests in that fixture without your having to declare them in > > > each test. I think of this as what a text fixture is for (collecting a set of > > > helper functions in one place that'll be easily available to a set of tests). > > > > > > Since the class needs to access a private nested class of HttpCache which is > > > HttpCache::Writers, it needs to itself be a nested class of HttpCache? > > > > Fair, though there are several ways around that. You could certainly make it a non-private class for testing, though that's got similar problems as to this approach. I would also think that creating a test fixture and making that a friend of both HttpCache and HttpCache::Writers would work, and would be in line with other existing test structuring. > > > > > And then > > > if I create as a fixture class the test with macro TEST_F does not take > > > HttpCache::WritersTest or WritersTest. Am I missing something here? > > > > I wouldn't expect it to anything with a :: in it--it would need to be (in the test file) a top level class. > > > > (I'm not dead set on the structures I'm pushing, but I do think that this is unusual, and the structures I'm pushing more in line with existing Chromium practice.) > > I will address this tomorrow but didn't want to delay uploading the other changes on this. It works on making the test class friends to both HttpCache and HttpCache::Writers. Thanks! Making it a friend class for only HttpCache did not work earlier.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Next round of comments. https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:115: rv = writers_->Read(bufs[i].get(), 256, callbacks[i].callback(), On 2017/06/26 20:45:34, shivanisha wrote: > On 2017/06/22 at 21:45:05, Randy Smith (Not in Mondays) wrote: > > On 2017/06/15 20:17:46, shivanisha wrote: > > > On 2017/06/14 at 19:49:17, Randy Smith (Not in Mondays) wrote: > > > > Worthwhile having a test with multiple writers with different sized > buffers? > > > > > > > > (Long term, after integration, we'll want to test that situation with them > > > reading different amounts, but I presume what'll happen then is the short > reads > > > will hit in cache when they come back, which I think is being implemented by > > > integration, not by this CL.) > > > > > > That's right, since the shorter buffer transaction will need to read the > cache, > > > that cannot be tested in this CL. > > > > Sorry, why? I understand why doing multiple reads on a single transaction > would need to read the cache, but why can't there be multiple transactions with > different buffer sizes? > > If there are two transactions reading simultaneously and the second has a > smaller buffer than the first, the second should be reading the delta data from > the cache but cache reading is part of the HC::T state machine and not writers > and should thus be tested in integration CL. Assuming in the following that the first is the active transaction/driving the network transaction. If the second has a *smaller* buffer than the first, it's reading a subset of what comes from the network, and so shouldn't touch the cache at all. If that analysis is right, I think it should be tested in this CL. If the second has a *larger* buffer, I could imagine two implementations. The first is that only the data returned by the network transaction is returned to the consumer (short read). The second is that the second immediately takes over as the active_transaction_ and drives a new network request. The first strikes me as more in line with other network stack patterns (return data early rather than waiting until you get a full buffer), and is what I was assuming in my comment. If that's wrong, help me understand why we don't return early? If it's right, I'm inclined to think we should test that in this CL. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:28: friend class WritersTest; nit, suggestion: I'm used to seeing friend declarations in the private section. I can't find anything about it in the style guide, so up to you, but my sense is that consistency would suggest the private section. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:210: rv = callbacks[j].WaitForResult(); Confused--this looks like we throw away all rvs except for the last. Should we be expecting/asserting consistency? (Or expected inconsistency?) If this routine is only expected to be used in cases in which the rvs will be positive (so that the first error means a test failure and we don't really care about what happens after), document that in a comment above the routine? (Dual comments applies to anywhere else in the file where we're stopping short on error rather than executing code for all the transactions.) https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:216: for (auto& result : results) { Missed a case on the "Increment i && use for (auto&" pattern? (https://codereview.chromium.org/2886483002/diff/440001/net/http/http_cache_wr...) https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:265: for (auto& result : results) { Missed a case on the "Increment i && use for (auto&" pattern? (https://codereview.chromium.org/2886483002/diff/440001/net/http/http_cache_wr...) https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:307: for (auto& callback : callbacks) { Missed a case on the "Increment i && use for (auto&" pattern? (https://codereview.chromium.org/2886483002/diff/440001/net/http/http_cache_wr...) https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:314: return rv; Is this because we don't expect to get an rv of zero in this routine? This seems like an "early return on success" case, which seems strange--if it's actually what's wanted, a comment as to why? https://codereview.chromium.org/2886483002/diff/560001/net/http/http_transact... File net/http/http_transaction_test_util.h (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_transact... net/http/http_transaction_test_util.h:291: // mode). This is part of the interface, not the implementation, and so should be in comments above the "private:" line. And I think this is moderately big deal (I tentatively put it in the category of "violation of the principle of least astonishment", meaning I think it'll take other users of this class by surprise in more cases than not), so I want to make sure it's called out reasonably loudly. Could you make sure that this is included in the doc for SetReadError() and a reference to it put in the comments before AddMockTransaction()? That's a bit unfair, since these classes are very much not well documented currently, but that makes me more worried about making them confusing to use, not less. Alternatively: I think this same behavior could be done by specifying a read_handler in the transaction passed to AddMockTransaction(). Any chance you could simply use that existing interface and not bother with having to document new behavior?
https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:36: writers_ = base::MakeUnique<HttpCache::Writers>(disk_entry); Sorry, missed this on the last round. Josh suggested using MockDiskEntry here, and I agree.
https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:36: writers_ = base::MakeUnique<HttpCache::Writers>(disk_entry); On 2017/06/27 at 18:50:56, Randy Smith (Not in Mondays) wrote: > Sorry, missed this on the last round. Josh suggested using MockDiskEntry here, and I agree. cache_.CreateBackendEntry is creating a MockDiskEntry and disk_cache::Entry is its abstract base class.
https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:36: writers_ = base::MakeUnique<HttpCache::Writers>(disk_entry); On 2017/06/27 19:30:41, shivanisha wrote: > On 2017/06/27 at 18:50:56, Randy Smith (Not in Mondays) wrote: > > Sorry, missed this on the last round. Josh suggested using MockDiskEntry > here, and I agree. > > cache_.CreateBackendEntry is creating a MockDiskEntry and disk_cache::Entry is > its abstract base class. Ooops. Ok, ignore this note :-}.
https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache.h File net/http/http_cache.h (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache.h#... net/http/http_cache.h:71: // class WritersTest; What's this? https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache.h#... net/http/http_cache.h:253: friend class WritersTest1; // To access ActiveEntry in the test class. What's this? https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:8: #include <utility> newline between groups of includes https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:23: DCHECK(buf); include file missing for DCHECK https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:34: return ERR_IO_PENDING; need include file for ERR_IO_PENDING https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:93: if (!transaction) Huh, do we really expect to have nullptr here? https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:98: DCHECK(it != all_writers_.end()); DCHECK_NE https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:99: all_writers_.erase(transaction); Better to erase the iterator. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:207: DCHECK(next_state_ != State::NONE); DCHECK_NE https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:211: next_state_ = State::NONE; Can you use the same STATE::INVALID as we do in HTTPCache::Transaction to make NONE transitions explicit? Thanks! https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:232: default: Better not to have a default case in this switch statement. That way if a new state is added to the enum we'll get a compile-time error here instead of a runtime error. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:289: // Todo(shivanisha): When partial requests support parallel writing, this s/Todo/TODO https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:416: transaction->SetSharedWritingFailState(result); I'm not sure that this new method is necessary. What if Read() just immediately fails if the transaction isn't in the set of active transactions? You could test it in this CL. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:19: // starve other consumers of the same resource. This should be a part of the HttpCache::Writers comment. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:23: // Writer represents the set of all HttpCache::Transactions that are s/Writer/Writers/ https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:28: friend class WritersTest; newline after the friend declaration https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:39: // run to inform the consumer of the result of the Read(). Comment doesn't make it clear what happens if Writers is destroyed while consumer is waiting for the callback to be called. Will it still be called? https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:46: // It stops caching only if there are no other transactions. Returns true if If there are two simultaneous downloads of the same large file, and both call StopCaching(), they won't actually stop, is that correct? https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:50: // Membership functions. Where does the scope of this comment end? I doubt that TruncateEntry is a membership function. I'd just remove the comment. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:54: // Should only be invoked if CanAddTransaction() returns true. Do you mean CanAddWriters? https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:59: // |transaction| can be destroyed at any point and it should invoke Should the last line read: Call RemoveTransaction before destroying |transaction|, it is safe to call at any time. ? https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:66: void RemoveTransaction(Transaction* transaction); When can this be called? https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:76: bool IsPresent(Transaction* transaction) const { suggest s/IsPresent/HasTransaction/ https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:88: HttpTransaction* network_transaction() { return network_transaction_.get(); } Interesting, who would need to call this function? https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:91: void TruncateEntry(); When can this be called? Even if a Read() is ongoing? https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:98: Remove newline. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:99: int CountTransactionsForTesting() const { return all_writers_.size(); } Are the ForTesting functions necessary? This class is friends with the testing class. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:28: ~WritersTest() override { cache_.disk_cache()->ReleaseAll(); } This call to ReleaseAll isn't necessary, the mock disk cache calls that in its destructor. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:31: // accessing those only in this class instead of in tests. This comment is kind of floating and I don't think it's really necessary, please remove. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:34: disk_cache::Entry* disk_entry = nullptr; You need to Close() each entry when you're done with it, else you'll have a leak. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:49: DCHECK_EQ(rv, OK); Avoid using DCHECK* macros in tests, as they crash the test binary and leave bots in a bad state. Use the ASSERT_xx() and EXPECT_xx() family of macros, which report failures gracefully and can continue running other tests. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:50: DCHECK(transaction.get()); s/transaction.get()/transaction/ https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:64: writers_->AddTransaction((HttpCache::Transaction*)(transaction.get()), This cast (here and elsewhere) makes me nervous, as there is no guarantee that MockHttpCache will return a HttpCache::Transaction. Instead, I suggest changing MockHttpCache::CreateTransaction to: int CreateTransaction(std::unique_ptr<HttpCache::Transaction>* trans); If, in the future, we make a mock transaction we can change it to that instead. Also note that when casting use static_cast<>() instead of C-style casts. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:107: int ReadAll(std::vector<std::string>& results) { output parameters should be raw pointers here and below https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:182: int ReadAllDeleteWaitingTransaction(std::vector<std::string>& results, output params come after input params here and below https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:397: I don't see a test for the public IsPresent function. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:400: CreateWritersAddTransaction(true); nit: true /* is_exclusive */ https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:417: It'd be good to add a test for UpdatePriority. Since WritersTest is a friend class of writers it could verify that priority_ is properly updated. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:446: EXPECT_EQ(expected, content); nit: seems reasonable to combine these two lines here and elsewhere: EXPECT_EQ(std::string(kSimpleGET_Transaction.data), content); https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:605: EXPECT_FALSE(writers_->IsEmpty()); These are the first two lines of every test. How about making them part of WritesTest initialization? https://codereview.chromium.org/2886483002/diff/560001/net/http/http_transact... File net/http/http_transaction_test_util.h (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_transact... net/http/http_transaction_test_util.h:246: void SetReadError(int error); What calls this? I don't see anything in the CL.
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/400001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:115: rv = writers_->Read(bufs[i].get(), 256, callbacks[i].callback(), On 2017/06/27 at 18:49:37, Randy Smith (Not in Mondays) wrote: > On 2017/06/26 20:45:34, shivanisha wrote: > > On 2017/06/22 at 21:45:05, Randy Smith (Not in Mondays) wrote: > > > On 2017/06/15 20:17:46, shivanisha wrote: > > > > On 2017/06/14 at 19:49:17, Randy Smith (Not in Mondays) wrote: > > > > > Worthwhile having a test with multiple writers with different sized > > buffers? > > > > > > > > > > (Long term, after integration, we'll want to test that situation with them > > > > reading different amounts, but I presume what'll happen then is the short > > reads > > > > will hit in cache when they come back, which I think is being implemented by > > > > integration, not by this CL.) > > > > > > > > That's right, since the shorter buffer transaction will need to read the > > cache, > > > > that cannot be tested in this CL. > > > > > > Sorry, why? I understand why doing multiple reads on a single transaction > > would need to read the cache, but why can't there be multiple transactions with > > different buffer sizes? > > > > If there are two transactions reading simultaneously and the second has a > > smaller buffer than the first, the second should be reading the delta data from > > the cache but cache reading is part of the HC::T state machine and not writers > > and should thus be tested in integration CL. > > Assuming in the following that the first is the active transaction/driving the network transaction. > > If the second has a *smaller* buffer than the first, it's reading a subset of what comes from the network, and so shouldn't touch the cache at all. If that analysis is right, I think it should be tested in this CL. > > If the second has a *larger* buffer, I could imagine two implementations. The first is that only the data returned by the network transaction is returned to the consumer (short read). The second is that the second immediately takes over as the active_transaction_ and drives a new network request. The first strikes me as more in line with other network stack patterns (return data early rather than waiting until you get a full buffer), and is what I was assuming in my comment. If that's wrong, help me understand why we don't return early? If it's right, I'm inclined to think we should test that in this CL. You are right, we can test that different buffer sizes in this CL for one Read call but not across multiple Read calls since the smaller buffer should read from the cache for the second Read() call. Added a test ReadMultipleDifferentBufferSizes to test this. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:28: friend class WritersTest; On 2017/06/27 at 18:49:37, Randy Smith (Not in Mondays) wrote: > nit, suggestion: I'm used to seeing friend declarations in the private section. I can't find anything about it in the style guide, so up to you, but my sense is that consistency would suggest the private section. moved to private section https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:210: rv = callbacks[j].WaitForResult(); On 2017/06/27 at 18:49:37, Randy Smith (Not in Mondays) wrote: > Confused--this looks like we throw away all rvs except for the last. Should we be expecting/asserting consistency? (Or expected inconsistency?) > > If this routine is only expected to be used in cases in which the rvs will be positive (so that the first error means a test failure and we don't really care about what happens after), document that in a comment above the routine? > > (Dual comments applies to anywhere else in the file where we're stopping short on error rather than executing code for all the transactions.) Added asserts to check that same rv is returned by all transactions https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:216: for (auto& result : results) { On 2017/06/27 at 18:49:37, Randy Smith (Not in Mondays) wrote: > Missed a case on the "Increment i && use for (auto&" pattern? (https://codereview.chromium.org/2886483002/diff/440001/net/http/http_cache_wr...) This was missed, changed now. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:265: for (auto& result : results) { On 2017/06/27 at 18:49:37, Randy Smith (Not in Mondays) wrote: > Missed a case on the "Increment i && use for (auto&" pattern? (https://codereview.chromium.org/2886483002/diff/440001/net/http/http_cache_wr...) Changed. Also did a search on all uses of auto to figure out all such cases. Thanks https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:307: for (auto& callback : callbacks) { On 2017/06/27 at 18:49:37, Randy Smith (Not in Mondays) wrote: > Missed a case on the "Increment i && use for (auto&" pattern? (https://codereview.chromium.org/2886483002/diff/440001/net/http/http_cache_wr...) Changed to use index in for loop https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:314: return rv; On 2017/06/27 at 18:49:37, Randy Smith (Not in Mondays) wrote: > Is this because we don't expect to get an rv of zero in this routine? This seems like an "early return on success" case, which seems strange--if it's actually what's wanted, a comment as to why? rv == 0 implies the end of reading by the active transaction and thus we return from here because there is nothing else to do. Changing this loop for readability. Changed the loop to be while (active_transaction_rv > 0) instead of while (true) and then we do not need this if condition https://codereview.chromium.org/2886483002/diff/560001/net/http/http_transact... File net/http/http_transaction_test_util.h (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_transact... net/http/http_transaction_test_util.h:291: // mode). On 2017/06/27 at 18:49:38, Randy Smith (Not in Mondays) wrote: > This is part of the interface, not the implementation, and so should be in comments above the "private:" line. And I think this is moderately big deal (I tentatively put it in the category of "violation of the principle of least astonishment", meaning I think it'll take other users of this class by surprise in more cases than not), so I want to make sure it's called out reasonably loudly. Could you make sure that this is included in the doc for SetReadError() and a reference to it put in the comments before AddMockTransaction()? That's a bit unfair, since these classes are very much not well documented currently, but that makes me more worried about making them confusing to use, not less. > > Alternatively: I think this same behavior could be done by specifying a read_handler in the transaction passed to AddMockTransaction(). Any chance you could simply use that existing interface and not bother with having to document new behavior? Added the error code in MockTransaction and now its definition and handling corresponds well with the existing return_code member. Renamed the existing member to start_return_code and this one to read_return_code
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Latest patch addresses Josh's feedback so all feedback till now is addressed. Also, it shows a dependency patchset because I had to do it for the subsequent CL but its not really a dependeny for this CL. I did the rebasing yesterday and feedback addressing today and in the context switch forgot to upload two different patches. Sorry about that. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache.h File net/http/http_cache.h (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache.h#... net/http/http_cache.h:71: // class WritersTest; On 2017/06/28 at 15:50:08, jkarlin_slow wrote: > What's this? Removed in a subsequent patch. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache.h#... net/http/http_cache.h:253: friend class WritersTest1; // To access ActiveEntry in the test class. On 2017/06/28 at 15:50:08, jkarlin_slow wrote: > What's this? Removed in a subsequent patch. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:8: #include <utility> On 2017/06/28 at 15:50:09, jkarlin_slow wrote: > newline between groups of includes Added https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:23: DCHECK(buf); On 2017/06/28 at 15:50:09, jkarlin_slow wrote: > include file missing for DCHECK Added logging.h https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:34: return ERR_IO_PENDING; On 2017/06/28 at 15:50:09, jkarlin_slow wrote: > need include file for ERR_IO_PENDING Added https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:93: if (!transaction) On 2017/06/28 at 15:50:09, jkarlin_slow wrote: > Huh, do we really expect to have nullptr here? Yes, it will be clearer in the integration CL. But the scenario I am expecting is: active_transaction_ is destroyed. cache write finishes and writers invoke DoneWritingToEntry with a null transaction which in turn invokes this function with a nullptr. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:98: DCHECK(it != all_writers_.end()); On 2017/06/28 at 15:50:09, jkarlin_slow wrote: > DCHECK_NE Gives a compile time error due to iterators being compared. Not sure how it is different with using != directly https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:99: all_writers_.erase(transaction); On 2017/06/28 at 15:50:09, jkarlin_slow wrote: > Better to erase the iterator. done https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:207: DCHECK(next_state_ != State::NONE); On 2017/06/28 at 15:50:09, jkarlin_slow wrote: > DCHECK_NE done https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:211: next_state_ = State::NONE; On 2017/06/28 at 15:50:09, jkarlin_slow wrote: > Can you use the same STATE::INVALID as we do in HTTPCache::Transaction to make NONE transitions explicit? Thanks! done https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:232: default: On 2017/06/28 at 15:50:08, jkarlin_slow wrote: > Better not to have a default case in this switch statement. That way if a new state is added to the enum we'll get a compile-time error here instead of a runtime error. done https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:289: // Todo(shivanisha): When partial requests support parallel writing, this On 2017/06/28 at 15:50:09, jkarlin_slow wrote: > s/Todo/TODO done https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:416: transaction->SetSharedWritingFailState(result); On 2017/06/28 at 15:50:08, jkarlin_slow wrote: > I'm not sure that this new method is necessary. What if Read() just immediately fails if the transaction isn't in the set of active transactions? > > You could test it in this CL. That would need the HC::T::Read() to invoke Writers::Read() but that would be an added assumption in the code that Writers should be alive at that time. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:19: // starve other consumers of the same resource. On 2017/06/28 at 15:50:10, jkarlin_slow wrote: > This should be a part of the HttpCache::Writers comment. moved https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:23: // Writer represents the set of all HttpCache::Transactions that are On 2017/06/28 at 15:50:10, jkarlin_slow wrote: > s/Writer/Writers/ done https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:39: // run to inform the consumer of the result of the Read(). On 2017/06/28 at 15:50:09, jkarlin_slow wrote: > Comment doesn't make it clear what happens if Writers is destroyed while consumer is waiting for the callback to be called. Will it still be called? It will not be invoked. Added in the comment https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:46: // It stops caching only if there are no other transactions. Returns true if On 2017/06/28 at 15:50:10, jkarlin_slow wrote: > If there are two simultaneous downloads of the same large file, and both call StopCaching(), they won't actually stop, is that correct? That's right https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:50: // Membership functions. On 2017/06/28 at 15:50:09, jkarlin_slow wrote: > Where does the scope of this comment end? I doubt that TruncateEntry is a membership function. I'd just remove the comment. Removed https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:54: // Should only be invoked if CanAddTransaction() returns true. On 2017/06/28 at 15:50:09, jkarlin_slow wrote: > Do you mean CanAddWriters? yes, updated https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:59: // |transaction| can be destroyed at any point and it should invoke On 2017/06/28 at 15:50:09, jkarlin_slow wrote: > Should the last line read: Call RemoveTransaction before destroying |transaction|, it is safe to call at any time. > > ? The existing statement is conveying the same meaning? (Avoiding changes if possible to avoid many merge conflicts with the subsequent CL) https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:66: void RemoveTransaction(Transaction* transaction); On 2017/06/28 at 15:50:09, jkarlin_slow wrote: > When can this be called? Anytime a transaction is destroyed.Added https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:76: bool IsPresent(Transaction* transaction) const { On 2017/06/28 at 15:50:09, jkarlin_slow wrote: > suggest s/IsPresent/HasTransaction/ done https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:88: HttpTransaction* network_transaction() { return network_transaction_.get(); } On 2017/06/28 at 15:50:09, jkarlin_slow wrote: > Interesting, who would need to call this function? HC::T might. If not, I will remove it from the integration CL. Added a TODO https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:91: void TruncateEntry(); On 2017/06/28 at 15:50:09, jkarlin_slow wrote: > When can this be called? Even if a Read() is ongoing? No, it has a dcheck to assert that https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:98: On 2017/06/28 at 15:50:09, jkarlin_slow wrote: > Remove newline. done https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:99: int CountTransactionsForTesting() const { return all_writers_.size(); } On 2017/06/28 at 15:50:09, jkarlin_slow wrote: > Are the ForTesting functions necessary? This class is friends with the testing class. The compiler still gives an error if private members accessed in the test body https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:28: ~WritersTest() override { cache_.disk_cache()->ReleaseAll(); } On 2017/06/28 at 15:50:10, jkarlin_slow wrote: > This call to ReleaseAll isn't necessary, the mock disk cache calls that in its destructor. Yes, removed it https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:31: // accessing those only in this class instead of in tests. On 2017/06/28 at 15:50:10, jkarlin_slow wrote: > This comment is kind of floating and I don't think it's really necessary, please remove. removed https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:34: disk_cache::Entry* disk_entry = nullptr; On 2017/06/28 at 15:50:10, jkarlin_slow wrote: > You need to Close() each entry when you're done with it, else you'll have a leak. done https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:49: DCHECK_EQ(rv, OK); On 2017/06/28 at 15:50:10, jkarlin_slow wrote: > Avoid using DCHECK* macros in tests, as they crash the test binary and leave bots in a bad state. Use the ASSERT_xx() and EXPECT_xx() family of macros, which report failures gracefully and can continue running other tests. done https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:50: DCHECK(transaction.get()); On 2017/06/28 at 15:50:10, jkarlin_slow wrote: > s/transaction.get()/transaction/ done https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:64: writers_->AddTransaction((HttpCache::Transaction*)(transaction.get()), On 2017/06/28 at 15:50:10, jkarlin_slow wrote: > This cast (here and elsewhere) makes me nervous, as there is no guarantee that MockHttpCache will return a HttpCache::Transaction. > > Instead, I suggest changing MockHttpCache::CreateTransaction to: > > int CreateTransaction(std::unique_ptr<HttpCache::Transaction>* trans); > > If, in the future, we make a mock transaction we can change it to that instead. > > Also note that when casting use static_cast<>() instead of C-style casts. But MockHttpCache::CreateTransaction invokes HttpCache::CreateTransaction which requires an HttpTransaction Changed to static_cast https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:107: int ReadAll(std::vector<std::string>& results) { On 2017/06/28 at 15:50:10, jkarlin_slow wrote: > output parameters should be raw pointers here and below done https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:182: int ReadAllDeleteWaitingTransaction(std::vector<std::string>& results, On 2017/06/28 at 15:50:10, jkarlin_slow wrote: > output params come after input params here and below done https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:397: On 2017/06/28 at 15:50:10, jkarlin_slow wrote: > I don't see a test for the public IsPresent function. Added in CreateWritersAddTransaction https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:400: CreateWritersAddTransaction(true); On 2017/06/28 at 15:50:10, jkarlin_slow wrote: > nit: true /* is_exclusive */ done https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:417: On 2017/06/28 at 15:50:10, jkarlin_slow wrote: > It'd be good to add a test for UpdatePriority. Since WritersTest is a friend class of writers it could verify that priority_ is properly updated. Added in RemoveIdleTransaction https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:605: EXPECT_FALSE(writers_->IsEmpty()); On 2017/06/28 at 15:50:10, jkarlin_slow wrote: > These are the first two lines of every test. How about making them part of WritesTest initialization? I feel it will make the test body less contained so one would have to look up in atleast two places to understand the test.
Next round. This is converging, but I don't know if I'll end up with time for another review tomorrow. If not, I'll cede to Josh's judgement for the rest of the review. https://codereview.chromium.org/2886483002/diff/600001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/600001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:48: net::RequestPriority priority = DEFAULT_PRIORITY) { The google style guide discourages default arguments (https://google.github.io/styleguide/cppguide.html#Default_Arguments ). I'm not determined to hold to the style guide for test code, but it's better to be consistent than not, so I'd suggest instead have two functions with different names, one that calls the other with the above arguments. Up to you. https://codereview.chromium.org/2886483002/diff/600001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:129: if (rv == ERR_IO_PENDING) { I don't think this test makes sense. If any of the rvs aren't ERR_IO_PENDING, the code below will hang since the callbacks won't be called, and this just tests the last one. I'd suggest tracking if any rv isn't ERR_IO_PENDING, returning that rv if so, and removing this conditional. https://codereview.chromium.org/2886483002/diff/600001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:131: for (size_t i = 1; i < callbacks.size(); i++) { nit: I think this fails if *results is of length 1 (rv isn't set properly below in that case). Could you either DCHECK that that's not true or change the logic here so it does work? https://codereview.chromium.org/2886483002/diff/600001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:163: int smaller_buffer_size = 10; Reasonable to also test the case where the smaller buffer size is first? In that case I'd expect the larger buffer to return with only the smaller buffer size read. https://codereview.chromium.org/2886483002/diff/600001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:210: if (rv == ERR_IO_PENDING) { Same comment as above--I don't think this makes sense, because the test is in trouble if any rv wasn't ERR_IO_PENDING, and this only tests the last one. (Applies at all other relevant locations in file.) https://codereview.chromium.org/2886483002/diff/600001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:216: for (size_t i = 2; i < callbacks.size(); i++) { Same comment as above--I think this fails (doesn't set rv properly) if callbacks.size() == 2. (Applies at all other relevant locations in file.)
https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:98: DCHECK(it != all_writers_.end()); On 2017/06/28 19:38:19, shivanisha wrote: > On 2017/06/28 at 15:50:09, jkarlin_slow wrote: > > DCHECK_NE > > Gives a compile time error due to iterators being compared. Not sure how it is > different with using != directly Good point, we don't actually need to print out the value of the iterator. Looks good as is. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:365: void HttpCache::Writers::OnCacheWriteFailure() { This is interesting. We actually expect the cache to fail writes from time to time. E.g., if the resource is large (> 1/8th max cache size), it will fail rather than fill the whole cache with one entry. So that means that anytime we have parallel fetches of a largish (~20MB on Android) resource, all but one of them is going to fail seemingly inexplicably. This worries me. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:416: transaction->SetSharedWritingFailState(result); On 2017/06/28 19:38:19, shivanisha wrote: > On 2017/06/28 at 15:50:08, jkarlin_slow wrote: > > I'm not sure that this new method is necessary. What if Read() just > immediately fails if the transaction isn't in the set of active transactions? > > > > You could test it in this CL. > > That would need the HC::T::Read() to invoke Writers::Read() but that would be an > added assumption in the code that Writers should be alive at that time. Just to make sure, but the idea is that HC::T::Read() will always invoke Writers::Read() when it actually tries to read right? So you're saying someone might call Read() on HC::T after the ActiveEntry (and Writers) have been destroyed? In that case can't HC::T:Read just just check for the existence of the writers object and abort if it's gone? https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:46: // It stops caching only if there are no other transactions. Returns true if On 2017/06/28 19:38:20, shivanisha wrote: > On 2017/06/28 at 15:50:10, jkarlin_slow wrote: > > If there are two simultaneous downloads of the same large file, and both call > StopCaching(), they won't actually stop, is that correct? > > That's right This needs to be clearly documented in HttpTransaction::StopCaching. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:91: void TruncateEntry(); On 2017/06/28 19:38:20, shivanisha wrote: > On 2017/06/28 at 15:50:09, jkarlin_slow wrote: > > When can this be called? Even if a Read() is ongoing? > > No, it has a dcheck to assert that Please update the comment to reflect when it can be called. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:64: writers_->AddTransaction((HttpCache::Transaction*)(transaction.get()), On 2017/06/28 19:38:21, shivanisha wrote: > On 2017/06/28 at 15:50:10, jkarlin_slow wrote: > > This cast (here and elsewhere) makes me nervous, as there is no guarantee that > MockHttpCache will return a HttpCache::Transaction. > > > > Instead, I suggest changing MockHttpCache::CreateTransaction to: > > > > int CreateTransaction(std::unique_ptr<HttpCache::Transaction>* trans); > > > > If, in the future, we make a mock transaction we can change it to that > instead. > > > > Also note that when casting use static_cast<>() instead of C-style casts. > > But MockHttpCache::CreateTransaction invokes HttpCache::CreateTransaction which > requires an HttpTransaction Ah.. because we're passing unique_ptrs to things around.. right. Given that it's a test it seems reasonable. > > Changed to static_cast https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:605: EXPECT_FALSE(writers_->IsEmpty()); On 2017/06/28 19:38:21, shivanisha wrote: > On 2017/06/28 at 15:50:10, jkarlin_slow wrote: > > These are the first two lines of every test. How about making them part of > WritesTest initialization? > > I feel it will make the test body less contained so one would have to look up in > atleast two places to understand the test. Okay, up to you.
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patch 23 addresses Randy's latest feedback. https://codereview.chromium.org/2886483002/diff/600001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/600001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:48: net::RequestPriority priority = DEFAULT_PRIORITY) { On 2017/06/29 at 00:53:49, Randy Smith (Not in Mondays) wrote: > The google style guide discourages default arguments (https://google.github.io/styleguide/cppguide.html#Default_Arguments ). I'm not determined to hold to the style guide for test code, but it's better to be consistent than not, so I'd suggest instead have two functions with different names, one that calls the other with the above arguments. Up to you. Created CreateWritersAddTransactionPriority which takes priority and passes it to CreateWritersAddTransaction. Keeping is_exclusive as default argument since it really is mostly false and set only in very few test cases. https://codereview.chromium.org/2886483002/diff/600001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:129: if (rv == ERR_IO_PENDING) { On 2017/06/29 at 00:53:50, Randy Smith (Not in Mondays) wrote: > I don't think this test makes sense. If any of the rvs aren't ERR_IO_PENDING, the code below will hang since the callbacks won't be called, and this just tests the last one. I'd suggest tracking if any rv isn't ERR_IO_PENDING, returning that rv if so, and removing this conditional. Since the EXPECT_EQ above asserts all rvs to be ERR_IO_PENDING, removing this if. Here and in all similar functions. https://codereview.chromium.org/2886483002/diff/600001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:131: for (size_t i = 1; i < callbacks.size(); i++) { On 2017/06/29 at 00:53:49, Randy Smith (Not in Mondays) wrote: > nit: I think this fails if *results is of length 1 (rv isn't set properly below in that case). Could you either DCHECK that that's not true or change the logic here so it does work? Changed the logic to have rv = prev_rv before the loop starts. Here and in all similar functions. https://codereview.chromium.org/2886483002/diff/600001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:163: int smaller_buffer_size = 10; On 2017/06/29 at 00:53:49, Randy Smith (Not in Mondays) wrote: > Reasonable to also test the case where the smaller buffer size is first? In that case I'd expect the larger buffer to return with only the smaller buffer size read. Added, also changed this function to work in both tests. https://codereview.chromium.org/2886483002/diff/600001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:210: if (rv == ERR_IO_PENDING) { On 2017/06/29 at 00:53:49, Randy Smith (Not in Mondays) wrote: > Same comment as above--I don't think this makes sense, because the test is in trouble if any rv wasn't ERR_IO_PENDING, and this only tests the last one. > > (Applies at all other relevant locations in file.) Changed. https://codereview.chromium.org/2886483002/diff/600001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:216: for (size_t i = 2; i < callbacks.size(); i++) { On 2017/06/29 at 00:53:49, Randy Smith (Not in Mondays) wrote: > Same comment as above--I think this fails (doesn't set rv properly) if callbacks.size() == 2. > > (Applies at all other relevant locations in file.) Changed
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #23 (id:620001) has been deleted
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patch 24 addresses Josh's latest feedback. Thanks! https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:365: void HttpCache::Writers::OnCacheWriteFailure() { On 2017/06/30 at 18:20:39, jkarlin_slow wrote: > This is interesting. We actually expect the cache to fail writes from time to time. E.g., if the resource is large (> 1/8th max cache size), it will fail rather than fill the whole cache with one entry. > > So that means that anytime we have parallel fetches of a largish (~20MB on Android) resource, all but one of them is going to fail seemingly inexplicably. > > This worries me. As discussed f2f, since only around 0.003% of simple cache backend entries fail to write due to to max size (as seen from UMA), the probability of having multiple simultaneous requests for such large entries will be lesser so failing them should be fine. For memory backends, since memory backend may hit this scenario more (a UMA is currently being added), we can disallow shared writing if the backend is memory, at least till the UMA is reported back. Will implement this restriction in a later CL. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:416: transaction->SetSharedWritingFailState(result); On 2017/06/30 at 18:20:38, jkarlin_slow wrote: > On 2017/06/28 19:38:19, shivanisha wrote: > > On 2017/06/28 at 15:50:08, jkarlin_slow wrote: > > > I'm not sure that this new method is necessary. What if Read() just > > immediately fails if the transaction isn't in the set of active transactions? > > > > > > You could test it in this CL. > > > > That would need the HC::T::Read() to invoke Writers::Read() but that would be an > > added assumption in the code that Writers should be alive at that time. > > Just to make sure, but the idea is that HC::T::Read() will always invoke Writers::Read() when it actually tries to read right? > > So you're saying someone might call Read() on HC::T after the ActiveEntry (and Writers) have been destroyed? In that case can't HC::T:Read just just check for the existence of the writers object and abort if it's gone? There are multiple scenarios in HC::T::Read() to decide how the read should proceed, including the following: 1. Part of writers - read from network 2. Part of writers - read from cache what is already read by another transaction 3. Not part of writers - read from network(has a valid network_transaction) - since the transaction was created with BYPASS_CACHE 4. Not part of writers - read from cache - since the response is already written to the cache and this is now part of entry_->readers. So either this failed scenario can be implied if none of the above 1,2,3 are true and it is also not part of entry_->readers or checking the state. I am fine either ways but slightly leaning towards a state since that is a clearer check. Also worth re-visiting in the integration CL. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:46: // It stops caching only if there are no other transactions. Returns true if On 2017/06/30 at 18:20:39, jkarlin_slow wrote: > On 2017/06/28 19:38:20, shivanisha wrote: > > On 2017/06/28 at 15:50:10, jkarlin_slow wrote: > > > If there are two simultaneous downloads of the same large file, and both call > > StopCaching(), they won't actually stop, is that correct? > > > > That's right > > This needs to be clearly documented in HttpTransaction::StopCaching. Yes, will document it in the integration CL. Added a TODO for it here https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.h:91: void TruncateEntry(); On 2017/06/30 at 18:20:39, jkarlin_slow wrote: > On 2017/06/28 19:38:20, shivanisha wrote: > > On 2017/06/28 at 15:50:09, jkarlin_slow wrote: > > > When can this be called? Even if a Read() is ongoing? > > > > No, it has a dcheck to assert that > > Please update the comment to reflect when it can be called. done
https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:365: void HttpCache::Writers::OnCacheWriteFailure() { On 2017/07/05 18:55:42, shivanisha wrote: > On 2017/06/30 at 18:20:39, jkarlin_slow wrote: > > This is interesting. We actually expect the cache to fail writes from time to > time. E.g., if the resource is large (> 1/8th max cache size), it will fail > rather than fill the whole cache with one entry. > > > > So that means that anytime we have parallel fetches of a largish (~20MB on > Android) resource, all but one of them is going to fail seemingly inexplicably. > > > > This worries me. > > As discussed f2f, since only around 0.003% of simple cache backend entries fail > to write due to to max size (as seen from UMA), the probability of having > multiple simultaneous requests for such large entries will be lesser so failing > them should be fine. > For memory backends, since memory backend may hit this scenario more (a UMA is > currently being added), we can disallow shared writing if the backend is memory, > at least till the UMA is reported back. Will implement this restriction in a > later CL. Acknowledged. https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:416: transaction->SetSharedWritingFailState(result); On 2017/07/05 18:55:42, shivanisha wrote: > On 2017/06/30 at 18:20:38, jkarlin_slow wrote: > > On 2017/06/28 19:38:19, shivanisha wrote: > > > On 2017/06/28 at 15:50:08, jkarlin_slow wrote: > > > > I'm not sure that this new method is necessary. What if Read() just > > > immediately fails if the transaction isn't in the set of active > transactions? > > > > > > > > You could test it in this CL. > > > > > > That would need the HC::T::Read() to invoke Writers::Read() but that would > be an > > > added assumption in the code that Writers should be alive at that time. > > > > Just to make sure, but the idea is that HC::T::Read() will always invoke > Writers::Read() when it actually tries to read right? > > > > So you're saying someone might call Read() on HC::T after the ActiveEntry (and > Writers) have been destroyed? In that case can't HC::T:Read just just check for > the existence of the writers object and abort if it's gone? > > There are multiple scenarios in HC::T::Read() to decide how the read should > proceed, including the following: > 1. Part of writers - read from network This will fail because the transaction isn't part of the Writers object > 2. Part of writers - read from cache what is already read by another transaction This will fail because the transaction isn't part of the Writers object > 3. Not part of writers - read from network(has a valid network_transaction) - > since the transaction was created with BYPASS_CACHE Can you go from being part of Writers to not being part of Writers? > 4. Not part of writers - read from cache - since the response is already written > to the cache and this is now part of entry_->readers. > > So either this failed scenario can be implied if none of the above 1,2,3 are > true and it is also not part of entry_->readers or checking the state. I am fine > either ways but slightly leaning towards a state since that is a clearer check. > Also worth re-visiting in the integration CL. Okay, let's revisit in the integration CL. https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers.h:22: Add a // on this line since it's part of the same comment block https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers.h:28: // |*disk_entry| must outlive this object. s/|*disk_entry|/|entry|/ https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers.h:152: Remove newline, the comment will apply to all methods underneath until the next empty line. https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers.h:174: bool ContainsOnlyIdleWriters() const; Define what idle writers are. https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:136: for (size_t i = 1; i < callbacks.size(); i++) { Really qequires at least 2 transactions, can you ASSERT that at the top of this function? Then you can remove the rv = prev_rv. https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:179: EXPECT_EQ(rv, ERR_IO_PENDING); // Since the default is asynchronous. In expect/assert/dcheck/check macros the argument you're expecting should come first, e.g., EXPECT_EQ(ERR_IO_PENDING, rv). It formats the output on failure such that the first argument is what was expected and the second is what it got. Please ensure that's done throughout this file. https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:191: } How about: std::vector<size_t> expected_lengths = { buffer_lengths[0], buffer_lengths[0] < buffer_lengths[1] ? buffer_lengths[0] : buffer_lengths[1] }; for (size_t i = 0; i < callbacks.size(); i++) { rv = callbacks[i].WaitForResult(); EXPECT_EQ(expected_lengths[i], rv); } https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:212: int ReadAllDeleteActiveTransaction(std::vector<std::string>* results) { Seems like the ReadAllDeleteXTransaction functions could be consolidated into one function and an argument that tells it which transaction to delete. And then ReadAll() would call ReadAllDeleteXTransaction with an argument of NONE. https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:237: for (size_t i = 2; i < callbacks.size(); i++) { This really requires at least 3 transactions, can you ASSERT that at the top of this function? Then you can remove the rv=prev_rv line. https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:452: // Tests successful addition of multiple transaction. s/transaction/transactions/ https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:463: // Tests that CanAddWriters should return false if it is exclusive writing. s/exclusive writing/writing exclusively/ https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:548: std::vector<int> buffer_lengths; std::vector<int> buffer_lengths {20, 10}; https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:563: std::vector<int> buffer_lengths; std::vector<int> buffer_lengths {10, 20};
https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/560001/net/http/http_cache_wr... net/http/http_cache_writers.cc:416: transaction->SetSharedWritingFailState(result); On 2017/07/06 18:50:04, jkarlin_slow wrote: > On 2017/07/05 18:55:42, shivanisha wrote: > > On 2017/06/30 at 18:20:38, jkarlin_slow wrote: > > > On 2017/06/28 19:38:19, shivanisha wrote: > > > > On 2017/06/28 at 15:50:08, jkarlin_slow wrote: > > > > > I'm not sure that this new method is necessary. What if Read() just > > > > immediately fails if the transaction isn't in the set of active > > transactions? > > > > > > > > > > You could test it in this CL. > > > > > > > > That would need the HC::T::Read() to invoke Writers::Read() but that would > > be an > > > > added assumption in the code that Writers should be alive at that time. > > > > > > Just to make sure, but the idea is that HC::T::Read() will always invoke > > Writers::Read() when it actually tries to read right? > > > > > > So you're saying someone might call Read() on HC::T after the ActiveEntry > (and > > Writers) have been destroyed? In that case can't HC::T:Read just just check > for > > the existence of the writers object and abort if it's gone? > > > > There are multiple scenarios in HC::T::Read() to decide how the read should > > proceed, including the following: > > 1. Part of writers - read from network > > This will fail because the transaction isn't part of the Writers object > > > 2. Part of writers - read from cache what is already read by another > transaction > > This will fail because the transaction isn't part of the Writers object > > > 3. Not part of writers - read from network(has a valid network_transaction) - > > since the transaction was created with BYPASS_CACHE > > Can you go from being part of Writers to not being part of Writers? > > > 4. Not part of writers - read from cache - since the response is already > written > > to the cache and this is now part of entry_->readers. > > > > So either this failed scenario can be implied if none of the above 1,2,3 are > > true and it is also not part of entry_->readers or checking the state. I am > fine > > either ways but slightly leaning towards a state since that is a clearer > check. > > Also worth re-visiting in the integration CL. > > Okay, let's revisit in the integration CL. Ignore my comments above, I meant to just post the last sentence, that we'll revisit later.
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
PTAL, Thanks! https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... File net/http/http_cache_writers.h (right): https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers.h:22: On 2017/07/06 at 18:50:04, jkarlin_slow wrote: > Add a // on this line since it's part of the same comment block done https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers.h:28: // |*disk_entry| must outlive this object. On 2017/07/06 at 18:50:04, jkarlin_slow wrote: > s/|*disk_entry|/|entry|/ done https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers.h:152: On 2017/07/06 at 18:50:04, jkarlin_slow wrote: > Remove newline, the comment will apply to all methods underneath until the next empty line. done https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers.h:174: bool ContainsOnlyIdleWriters() const; On 2017/07/06 at 18:50:04, jkarlin_slow wrote: > Define what idle writers are. done https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:136: for (size_t i = 1; i < callbacks.size(); i++) { On 2017/07/06 at 18:50:04, jkarlin_slow wrote: > Really qequires at least 2 transactions, can you ASSERT that at the top of this function? Then you can remove the rv = prev_rv. done https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:179: EXPECT_EQ(rv, ERR_IO_PENDING); // Since the default is asynchronous. On 2017/07/06 at 18:50:04, jkarlin_slow wrote: > In expect/assert/dcheck/check macros the argument you're expecting should come first, e.g., EXPECT_EQ(ERR_IO_PENDING, rv). It formats the output on failure such that the first argument is what was expected and the second is what it got. > > Please ensure that's done throughout this file. done https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:191: } On 2017/07/06 at 18:50:04, jkarlin_slow wrote: > How about: > > std::vector<size_t> expected_lengths = { buffer_lengths[0], buffer_lengths[0] < buffer_lengths[1] ? buffer_lengths[0] : buffer_lengths[1] }; > > for (size_t i = 0; i < callbacks.size(); i++) { > rv = callbacks[i].WaitForResult(); > EXPECT_EQ(expected_lengths[i], rv); > } Much nicer, thanks. https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:212: int ReadAllDeleteActiveTransaction(std::vector<std::string>* results) { On 2017/07/06 at 18:50:04, jkarlin_slow wrote: > Seems like the ReadAllDeleteXTransaction functions could be consolidated into one function and an argument that tells it which transaction to delete. And then ReadAll() would call ReadAllDeleteXTransaction with an argument of NONE. Done. Happy to remove all the other functions. https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:237: for (size_t i = 2; i < callbacks.size(); i++) { On 2017/07/06 at 18:50:04, jkarlin_slow wrote: > This really requires at least 3 transactions, can you ASSERT that at the top of this function? Then you can remove the rv=prev_rv line. done https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:452: // Tests successful addition of multiple transaction. On 2017/07/06 at 18:50:04, jkarlin_slow wrote: > s/transaction/transactions/ done https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:463: // Tests that CanAddWriters should return false if it is exclusive writing. On 2017/07/06 at 18:50:04, jkarlin_slow wrote: > s/exclusive writing/writing exclusively/ done https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:548: std::vector<int> buffer_lengths; On 2017/07/06 at 18:50:04, jkarlin_slow wrote: > std::vector<int> buffer_lengths {20, 10}; done https://codereview.chromium.org/2886483002/diff/660001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:563: std::vector<int> buffer_lengths; On 2017/07/06 at 18:50:04, jkarlin_slow wrote: > std::vector<int> buffer_lengths {10, 20}; done
LGTM conditional on notes below on checking data read and one resolution or another to the EXPECT/ASSERT tension; everything else up to you. https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:105: scoped_refptr<IOBuffer> buf(new IOBuffer(256)); nit, suggestion: 256->constant. https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:122: int ReadAll() { nit, suggestion: I'd find this more readable if ReadAll() was defined immediately after ReadAllDeleteTransaction(). https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:170: EXPECT_EQ(results[0].substr(0, expected_lengths[1]), results[1]); So it took me a moment to recognize that this was depending on expected_lengths[1] <= expected_lengths[0] in all cases. Given that that's true (i.e. that the code reader needs to puzzle over the code), why bother with the conditional? I believe this EXPECT_EQ is equivalent to the one above on line 168 if expected_lengths[0] == expected_lengths[0]. https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:171: } Since we're doing copying (i.e. touching the data) can we also check that the data read is accurate? https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:182: delete_index = 0; Why set it explicitly for this value and not for NONE? (Given that it's being set to the default here?) https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:201: if (!first_iter && deleteType != DeleteTransactionType::NONE && nit, suggestion: Explanatory comment? https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:222: rv = callbacks[i].WaitForResult(); nit, suggestion: I personally wouldn't bother with the intermediate assignment to rv. (Dependent on some nits & suggestions below.) https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:228: EXPECT_EQ(rvs[i - 1], rvs[i]); Given that the code below is pretty clearly assuming that all the rvs will be the same, I'm tempted to suggest this be an ASSERT rather than an EXPECT, but up to you. https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:231: if (rv > 0) { Given that the above is an EXPECT_EQ rather than an ASSERT_EQ, the code here can't make the assumption that all of the rvs actually were the same. Given that, I'd suggest explicitly referring to rv[rvs.size()-1] just so the code reader can see directly what "rv" is being used.
https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1163: // first transaction of that new entry and thus it should not be subject should not be subject to any cache lock delays. Are you confident that it won't be? In which case say, "will not have cache lock delays". Otherwise, we should probably have the timer? https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers.cc:245: } To enforce that the next state is set: DCHECK(next_state_ != STATE_UNSET) << "Previous state was " << state; https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:24: class MockHttpCacheTransaction : public HttpCache::Transaction { What's the purpose of this class? Generally Mocks don't actually implement anything, but this is a copy of HttpCache::Transaction with no changes. https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:156: std::vector<size_t> expected_lengths = { Why size_t? buffer_lengths is int, and you need it for the EXPECT_EQ below. https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:163: EXPECT_EQ(expected_lengths[i], (size_t)rv); static_cast, but you shouldn't need to cat if you make expected_lengths an int vector. https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:167: if (expected_lengths[0] == expected_lengths[1]) { Why these checks? They're redundant with expected_lengths checks above. https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:174: int ReadAllDeleteTransaction(DeleteTransactionType deleteType) { This method could use a comment https://codereview.chromium.org/2886483002/diff/720001/net/http/http_transact... File net/http/http_transaction_test_util.cc (right): https://codereview.chromium.org/2886483002/diff/720001/net/http/http_transact... net/http/http_transaction_test_util.cc:339: // Return immediately if we're returning an error. This comment isn't quite right, as there is no early return in this code.
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Randy! Addressed all feedback and rebased with main (in separate patches). https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1163: // first transaction of that new entry and thus it should not be subject On 2017/07/12 at 15:05:26, jkarlin wrote: > should not be subject to any cache lock delays. > > Are you confident that it won't be? In which case say, "will not have cache lock delays". Otherwise, we should probably have the timer? Yes, it should always be the writer of this entry. We also handle ERR_CACHE_RACE case while creating the entry so it should be always the case. Changed the comment Also, this change actually belongs to CL: https://codereview.chromium.org/2774603003. Not sure why its showing in the diffs here. Will rebase from master now that its already landed. https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers.cc:245: } On 2017/07/12 at 15:05:26, jkarlin wrote: > To enforce that the next state is set: > > DCHECK(next_state_ != STATE_UNSET) << "Previous state was " << state; Because state is a class enum, getting a compile error while printing it. Rest of the dcheck should be handled by the case State::UNSET above. https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:24: class MockHttpCacheTransaction : public HttpCache::Transaction { On 2017/07/12 at 15:05:26, jkarlin wrote: > What's the purpose of this class? Generally Mocks don't actually implement anything, but this is a copy of HttpCache::Transaction with no changes. Adding the overridden empty definitions. Its more helpful in the integration CLs for the unit tests to work correctly but I found it worth adding in this CL to get rid of the static casts from HttpTransaction to HttpCache::Transaction. https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:105: scoped_refptr<IOBuffer> buf(new IOBuffer(256)); On 2017/07/11 at 18:51:33, Randy Smith (Not in Mondays) wrote: > nit, suggestion: 256->constant. added static const int kDefaultBufferSize = 256 https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:122: int ReadAll() { On 2017/07/11 at 18:51:33, Randy Smith (Not in Mondays) wrote: > nit, suggestion: I'd find this more readable if ReadAll() was defined immediately after ReadAllDeleteTransaction(). Moved https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:156: std::vector<size_t> expected_lengths = { On 2017/07/12 at 15:05:26, jkarlin wrote: > Why size_t? buffer_lengths is int, and you need it for the EXPECT_EQ below. Changed to int. https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:163: EXPECT_EQ(expected_lengths[i], (size_t)rv); On 2017/07/12 at 15:05:26, jkarlin wrote: > static_cast, but you shouldn't need to cat if you make expected_lengths an int vector. Done https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:167: if (expected_lengths[0] == expected_lengths[1]) { On 2017/07/12 at 15:05:26, jkarlin wrote: > Why these checks? They're redundant with expected_lengths checks above. Replaced with just one check: EXPECT_EQ(results[0].substr(0, expected_lengths[1]), results[1]); https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:170: EXPECT_EQ(results[0].substr(0, expected_lengths[1]), results[1]); On 2017/07/11 at 18:51:33, Randy Smith (Not in Mondays) wrote: > So it took me a moment to recognize that this was depending on expected_lengths[1] <= expected_lengths[0] in all cases. Given that that's true (i.e. that the code reader needs to puzzle over the code), why bother with the conditional? I believe this EXPECT_EQ is equivalent to the one above on line 168 if expected_lengths[0] == expected_lengths[0]. Replaced with just one check: EXPECT_EQ(results[0].substr(0, expected_lengths[1]), results[1]); https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:171: } On 2017/07/11 at 18:51:33, Randy Smith (Not in Mondays) wrote: > Since we're doing copying (i.e. touching the data) can we also check that the data read is accurate? done https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:174: int ReadAllDeleteTransaction(DeleteTransactionType deleteType) { On 2017/07/12 at 15:05:26, jkarlin wrote: > This method could use a comment done https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:182: delete_index = 0; On 2017/07/11 at 18:51:33, Randy Smith (Not in Mondays) wrote: > Why set it explicitly for this value and not for NONE? (Given that it's being set to the default here?) Changed the default value to std::numeric_limits<int>::max() for NONE to differentiate it from 0 for ACTIVE. https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:201: if (!first_iter && deleteType != DeleteTransactionType::NONE && On 2017/07/11 at 18:51:33, Randy Smith (Not in Mondays) wrote: > nit, suggestion: Explanatory comment? done. https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:228: EXPECT_EQ(rvs[i - 1], rvs[i]); On 2017/07/11 at 18:51:33, Randy Smith (Not in Mondays) wrote: > Given that the code below is pretty clearly assuming that all the rvs will be the same, I'm tempted to suggest this be an ASSERT rather than an EXPECT, but up to you. Changed to an ASSERT and also had to change the return type of the function to void for using an ASSERT since ASSERT returns from the function and the value of rv is now verified to be OK at the end of the function instead of returning. https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:231: if (rv > 0) { On 2017/07/11 at 18:51:33, Randy Smith (Not in Mondays) wrote: > Given that the above is an EXPECT_EQ rather than an ASSERT_EQ, the code here can't make the assumption that all of the rvs actually were the same. Given that, I'd suggest explicitly referring to rv[rvs.size()-1] just so the code reader can see directly what "rv" is being used. Changed to assert above https://codereview.chromium.org/2886483002/diff/720001/net/http/http_transact... File net/http/http_transaction_test_util.cc (right): https://codereview.chromium.org/2886483002/diff/720001/net/http/http_transact... net/http/http_transaction_test_util.cc:339: // Return immediately if we're returning an error. On 2017/07/12 at 15:05:26, jkarlin wrote: > This comment isn't quite right, as there is no early return in this code. removed
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm with comments https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... File net/http/http_cache_writers.cc (right): https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers.cc:245: } On 2017/07/13 18:35:50, shivanisha wrote: > On 2017/07/12 at 15:05:26, jkarlin wrote: > > To enforce that the next state is set: > > > > DCHECK(next_state_ != STATE_UNSET) << "Previous state was " << state; > > Because state is a class enum, getting a compile error while printing it. Rest > of the dcheck should be handled by the case State::UNSET above. Ah yes, the unset state does it. Thanks. https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:24: class MockHttpCacheTransaction : public HttpCache::Transaction { On 2017/07/13 18:35:50, shivanisha wrote: > On 2017/07/12 at 15:05:26, jkarlin wrote: > > What's the purpose of this class? Generally Mocks don't actually implement > anything, but this is a copy of HttpCache::Transaction with no changes. > > Adding the overridden empty definitions. > > Its more helpful in the integration CLs for the unit tests to work correctly but > I found it worth adding in this CL to get rid of the static casts from > HttpTransaction to HttpCache::Transaction. Okay, let's rename it then as it's not actually a mock. Perhaps TestHttpCacheTransaction? https://codereview.chromium.org/2886483002/diff/760001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/760001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:248: // transaction is deleted and all transactions complete reading the data. This comment doesn't describe the main intent, which is for all of the transactions to read the data. Suggest: Reads the network transaction with each transaction in turn in small chunks. If |deleteType| is not NONE, then it deletes the transaction of given type during the read process
Thanks Josh! https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/720001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:24: class MockHttpCacheTransaction : public HttpCache::Transaction { On 2017/07/14 at 18:30:48, jkarlin wrote: > On 2017/07/13 18:35:50, shivanisha wrote: > > On 2017/07/12 at 15:05:26, jkarlin wrote: > > > What's the purpose of this class? Generally Mocks don't actually implement > > anything, but this is a copy of HttpCache::Transaction with no changes. > > > > Adding the overridden empty definitions. > > > > Its more helpful in the integration CLs for the unit tests to work correctly but > > I found it worth adding in this CL to get rid of the static casts from > > HttpTransaction to HttpCache::Transaction. > > Okay, let's rename it then as it's not actually a mock. Perhaps TestHttpCacheTransaction? As discussed f2f, lets keep the name as Mock since the latest patch overrides the empty definitions. A subsequent CL will make sure that it only inherits an abstract class rather than HttpCache::Transaction. https://codereview.chromium.org/2886483002/diff/760001/net/http/http_cache_wr... File net/http/http_cache_writers_unittest.cc (right): https://codereview.chromium.org/2886483002/diff/760001/net/http/http_cache_wr... net/http/http_cache_writers_unittest.cc:248: // transaction is deleted and all transactions complete reading the data. On 2017/07/14 at 18:30:48, jkarlin wrote: > This comment doesn't describe the main intent, which is for all of the transactions to read the data. > > Suggest: > > Reads the network transaction with each transaction in turn in small chunks. If |deleteType| is not NONE, then it deletes the transaction of given type during the read process done
The CQ bit was checked by shivanisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org, jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/2886483002/#ps800001 (title: "Comment changed.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 800001, "attempt_start_ts": 1500065482763810, "parent_rev": "2d8b08c72cac89549268a67996c387323895ce22", "commit_rev": "c6582e1924c76f68e2b8873712cdf26a0b0c1edb"}
Message was sent while issue was closed.
Description was changed from ========== This CL adds a new class HttpCache::Writers which will implement multiple cache transactions reading from the network and writing to the cache enabling each of those transactions to drive reading the response body from the network. This ensures that a slow consumer does not starve other consumers of the same resource. This CL implements the Writers class in isolation and does not create an instance or integrates it with HttpCache/HttpCache::Transaction. In a follow up CL which will be dependent on parallel validation CL (https://codereview.chromium.org/2721933002/), a Writer object will replace entry- >writer. In case of transaction types that do not support shared writing like partial transactions, Writers will only contain one transaction at a time and will have is_exclusive set to true. The flow of Read() call will be: URLRequestHttpJob -> HttpCache::Transaction::Read() -> HttpCache::Writers::Read() -> HttpNetworkTransaction::Read() HttpCache::Writers will invoke HC::T's callback after writing the read buffer to the cache successfully or on network read failure/ cache write failure. So, in essence, HttpCache::Writers::Read() abstracts away the DoNetworkRead* and DoCacheWriteData* functions of HC::T HttpCache::Transaction that has invoked Read() is the |active_transaction_| and may be killed at any time by it's consumer. When a transaction completes writing to the cache, either failure or success, it invokes HttpCache::DoneWritingToEntry as it does now which will invoke the required functionality in Writers. But If |active_transaction_| does not exist, Writers object invokes HttpCache::DoneWritingToEntry itself. BUG=472740 ========== to ========== This CL adds a new class HttpCache::Writers which will implement multiple cache transactions reading from the network and writing to the cache enabling each of those transactions to drive reading the response body from the network. This ensures that a slow consumer does not starve other consumers of the same resource. This CL implements the Writers class in isolation and does not create an instance or integrates it with HttpCache/HttpCache::Transaction. In a follow up CL which will be dependent on parallel validation CL (https://codereview.chromium.org/2721933002/), a Writer object will replace entry- >writer. In case of transaction types that do not support shared writing like partial transactions, Writers will only contain one transaction at a time and will have is_exclusive set to true. The flow of Read() call will be: URLRequestHttpJob -> HttpCache::Transaction::Read() -> HttpCache::Writers::Read() -> HttpNetworkTransaction::Read() HttpCache::Writers will invoke HC::T's callback after writing the read buffer to the cache successfully or on network read failure/ cache write failure. So, in essence, HttpCache::Writers::Read() abstracts away the DoNetworkRead* and DoCacheWriteData* functions of HC::T HttpCache::Transaction that has invoked Read() is the |active_transaction_| and may be killed at any time by it's consumer. When a transaction completes writing to the cache, either failure or success, it invokes HttpCache::DoneWritingToEntry as it does now which will invoke the required functionality in Writers. But If |active_transaction_| does not exist, Writers object invokes HttpCache::DoneWritingToEntry itself. BUG=472740 Review-Url: https://codereview.chromium.org/2886483002 Cr-Commit-Position: refs/heads/master@{#486907} Committed: https://chromium.googlesource.com/chromium/src/+/c6582e1924c76f68e2b8873712cd... ==========
Message was sent while issue was closed.
Committed patchset #31 (id:800001) as https://chromium.googlesource.com/chromium/src/+/c6582e1924c76f68e2b8873712cd... |