Issue 1230113012: [net] Better StopCaching() handling for HttpCache::Transaction.

asanka

asanka@chromium.org changed reviewers: + rvargas@chromium.org

5 years, 5 months ago (2015-07-16 01:03:49 UTC) #1

asanka

PTAL? hubbe: FYI since you seem to be working on the same files.

5 years, 5 months ago (2015-07-16 01:52:50 UTC) #2

asanka

On 2015/07/16 at 01:52:50, asanka wrote: > PTAL? > > hubbe: FYI since you seem ...

5 years, 5 months ago (2015-07-16 13:26:13 UTC) #3

hubbe

https://codereview.chromium.org/1230113012/diff/40001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/1230113012/diff/40001/net/http/http_cache_transaction.cc#newcode2657 net/http/http_cache_transaction.cc:2657: DCHECK_EQ(STATE_NONE, next_state_); Shouldn't these DCHECKs go first? https://codereview.chromium.org/1230113012/diff/40001/net/http/http_cache_transaction.h File ...

5 years, 5 months ago (2015-07-16 15:56:12 UTC) #5

asanka

https://codereview.chromium.org/1230113012/diff/40001/net/http/http_cache_unittest.cc File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/1230113012/diff/40001/net/http/http_cache_unittest.cc#newcode7080 net/http/http_cache_unittest.cc:7080: // The only thing we can test right now. ...

5 years, 5 months ago (2015-07-16 16:16:58 UTC) #6

hubbe

https://codereview.chromium.org/1230113012/diff/40001/net/http/http_cache_unittest.cc File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/1230113012/diff/40001/net/http/http_cache_unittest.cc#newcode7080 net/http/http_cache_unittest.cc:7080: // The only thing we can test right now. ...

5 years, 5 months ago (2015-07-16 16:57:53 UTC) #7

asanka

https://codereview.chromium.org/1230113012/diff/40001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/1230113012/diff/40001/net/http/http_cache_transaction.cc#newcode2657 net/http/http_cache_transaction.cc:2657: DCHECK_EQ(STATE_NONE, next_state_); On 2015/07/16 at 15:56:12, hubbe wrote: > ...

5 years, 5 months ago (2015-07-16 17:57:00 UTC) #8

https://codereview.chromium.org/1230113012/diff/40001/net/http/http_cache_tra...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/1230113012/diff/40001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:2657: DCHECK_EQ(STATE_NONE, next_state_);
On 2015/07/16 at 15:56:12, hubbe wrote:
> Shouldn't these DCHECKs go first?

Moved up.

https://codereview.chromium.org/1230113012/diff/40001/net/http/http_cache_tra...
File net/http/http_cache_transaction.h (right):

https://codereview.chromium.org/1230113012/diff/40001/net/http/http_cache_tra...
net/http/http_cache_transaction.h:378: // Called when the entire request has
successfully finished reading. May start
On 2015/07/16 at 15:56:12, hubbe wrote:
> Please specify if you mean that the client has finished reading from us, or if
we have finished reading from the network.  I think it means the first, I just
want to make it explicit.

Done.

https://codereview.chromium.org/1230113012/diff/40001/net/http/http_cache_uni...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/1230113012/diff/40001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:7080: // The only thing we can test right now.
Ideally we would also verify that
On 2015/07/16 at 16:57:52, hubbe wrote:
> On 2015/07/16 16:16:58, asanka wrote:
> > On 2015/07/16 at 15:56:12, hubbe wrote:
> > > This comment makes no sense to me.
> > > Verify A, but currently !A?
> > > If so, perhaps we should file a  bug to fix A and put a link to it here?
> > 
> > The comment is explaining why we aren't checking more things.
> > 
> > - The expectation below is the only thing we can currently verify.
> > 
> > - Currently the code makes multiple requests which look like the following:
> >   * the single byte at the start of the uncached region.
> >   * the uncached region in 2GB chunks.
> > 
> >   Optimally we'd send a single request for the entire uncached range. But
that's
> > not currently possible until we have plumb int64_t all the way through the
> > stack, and reworking PartialData implementation. Once that happens, this
test
> > and others could add expectations that the specific predictable and correct
> > network request was generated by the cache transaction.
> > 
> >   I didn't want to add expectations for the current behavior since that
makes it
> > seem like we want to preserve that behavior. Hence the comment.
> > 
> > Does that make sense?
> 
> Yes, this is a much better explanation than what is in the comment, please add
this explanation to the comment.

Done.

rvargas (doing something else)

I've been looking at the bug report and I think that the bug is somewhere ...

5 years, 5 months ago (2015-07-16 19:22:56 UTC) #9

asanka

On 2015/07/16 at 19:22:56, rvargas wrote: > I've been looking at the bug report and ...

5 years, 5 months ago (2015-07-16 21:05:23 UTC) #10

On 2015/07/16 at 19:22:56, rvargas wrote:
> I've been looking at the bug report and I think that the bug is somewhere
else, with a caveat*.
> 
> The resource in question is not sparse, it is simply truncated, probably by a
previous download attempt.
> 
> In that case, we try to see if the server is ok with ranges, and then ask for
"the rest" of the request. The first issue that I see is that we are not asking
for a 64-bit range (or just [last-]) and instead we are truncating the range end
to 32-bits (GetNextRangeLen). Then the cache is "in the loop" for issuing
another request after that :(.
> 
> We should change the code so that we either ask for 64 bit ranges, or simply
an open end request for truncated entries, so that we can get out of the way
later.
>
> 
> *. The first caveat is of course that an entry could be sparse and not just
truncated. I still lean towards getting completely out of the way for downloads
though... otherwise we have another weird flow of reading from the cache,
issuing multiple network requests and not writing the data to the cache. By the
way, wasn't the long term plan to have the downloads code use range request to
deal with partial downloads? That sounds cleaner.
> 
> The second caveat is that even if we ask for [last-], if we have a significant
part of the resource, StopCaching can come before we finish serving data from
the cache (aka, before we ask for [last-]). But I still don't want to be in the
loop anymore :(
>

Understood. And I agree that we should try to get the cache transaction out of
the loop as soon as possible.

However, we can't do so at the instant at which StopCaching() is called. We'd
still need to crank the state machine until we get to a point where we can
guarantee that all the remaining bytes are going to come from the network.

How about this:
 - When StopCaching() is called, set up a flag without changing mode_.
 - At the completion of the next network read:
   - If caching is to be stopped.
     - If the network transaction is responsible for all remaining bytes: [1]
       - Truncate cache entry or drop it as appropriate.
       - Release cache entry.
       - Switch to passthrough for remainder.
     - Else:
       - Skip writing, and continue with rest of state machine.

With the exception of [1], the rest is implemented in this CL.

I can also add the open ended range request logic which would mean that the
transaction will hit the condition in [1] sooner.

What do you think?
 
> Also, this bug report (crbug/347458) and patch
(https://codereview.chromium.org/181173003/) are also relevant for what
StopCaching should do.

Yup. I think [1] will address this.

rvargas (doing something else)

On 2015/07/16 21:05:23, asanka wrote: > On 2015/07/16 at 19:22:56, rvargas wrote: > > I've ...

5 years, 5 months ago (2015-07-16 22:59:29 UTC) #11

On 2015/07/16 21:05:23, asanka wrote:
> On 2015/07/16 at 19:22:56, rvargas wrote:
> > I've been looking at the bug report and I think that the bug is somewhere
> else, with a caveat*.
> > 
> > The resource in question is not sparse, it is simply truncated, probably by
a
> previous download attempt.
> > 
> > In that case, we try to see if the server is ok with ranges, and then ask
for
> "the rest" of the request. The first issue that I see is that we are not
asking
> for a 64-bit range (or just [last-]) and instead we are truncating the range
end
> to 32-bits (GetNextRangeLen). Then the cache is "in the loop" for issuing
> another request after that :(.
> > 
> > We should change the code so that we either ask for 64 bit ranges, or simply
> an open end request for truncated entries, so that we can get out of the way
> later.
> >
> > 
> > *. The first caveat is of course that an entry could be sparse and not just
> truncated. I still lean towards getting completely out of the way for
downloads
> though... otherwise we have another weird flow of reading from the cache,
> issuing multiple network requests and not writing the data to the cache. By
the
> way, wasn't the long term plan to have the downloads code use range request to
> deal with partial downloads? That sounds cleaner.
> > 
> > The second caveat is that even if we ask for [last-], if we have a
significant
> part of the resource, StopCaching can come before we finish serving data from
> the cache (aka, before we ask for [last-]). But I still don't want to be in
the
> loop anymore :(
> >
> 
> Understood. And I agree that we should try to get the cache transaction out of
> the loop as soon as possible.
> 
> However, we can't do so at the instant at which StopCaching() is called. We'd
> still need to crank the state machine until we get to a point where we can
> guarantee that all the remaining bytes are going to come from the network.
> 
> How about this:
>  - When StopCaching() is called, set up a flag without changing mode_.
>  - At the completion of the next network read:
>    - If caching is to be stopped.
>      - If the network transaction is responsible for all remaining bytes: [1]
>        - Truncate cache entry or drop it as appropriate.
>        - Release cache entry.
>        - Switch to passthrough for remainder.
>      - Else:
>        - Skip writing, and continue with rest of state machine.
> 
> With the exception of [1], the rest is implemented in this CL.
> 
> I can also add the open ended range request logic which would mean that the
> transaction will hit the condition in [1] sooner.
> 
> What do you think?
>  
> > Also, this bug report (crbug/347458) and patch
> (https://codereview.chromium.org/181173003/) are also relevant for what
> StopCaching should do.
> 
> Yup. I think [1] will address this.

That only partially addresses bug 347458 as it will keep the transaction locked
indefinitely (gated on the first download) while we wait to get to a place when
we can drop things.

Counter proposal:

- Formalize that StopCaching behaves like other state changing methods of a
request, and cannot be called while Start or Read are in progress.

- When StopCaching is received abort any operations right away:
  - If no ranges are involved || (truncated_ and reading after cached content) :
    - Fire and forget a write to truncate entry and release entry_
  - if truncated_ and reading before cached content end || sparse_ (aka else)
    - fire and forget write to truncate (if needed)
    - release entry_
    - delete network_transaction
    - create network_transaction bytes:current-
      - make sure to not return the headers again.

asanka

On 2015/07/16 at 22:59:29, rvargas wrote: > On 2015/07/16 21:05:23, asanka wrote: > > On ...

5 years, 5 months ago (2015-07-17 00:08:33 UTC) #12

On 2015/07/16 at 22:59:29, rvargas wrote:
> On 2015/07/16 21:05:23, asanka wrote:
> > On 2015/07/16 at 19:22:56, rvargas wrote:
> > > I've been looking at the bug report and I think that the bug is somewhere
> > else, with a caveat*.
> > > 
> > > The resource in question is not sparse, it is simply truncated, probably
by a
> > previous download attempt.
> > > 
> > > In that case, we try to see if the server is ok with ranges, and then ask
for
> > "the rest" of the request. The first issue that I see is that we are not
asking
> > for a 64-bit range (or just [last-]) and instead we are truncating the range
end
> > to 32-bits (GetNextRangeLen). Then the cache is "in the loop" for issuing
> > another request after that :(.
> > > 
> > > We should change the code so that we either ask for 64 bit ranges, or
simply
> > an open end request for truncated entries, so that we can get out of the way
> > later.
> > >
> > > 
> > > *. The first caveat is of course that an entry could be sparse and not
just
> > truncated. I still lean towards getting completely out of the way for
downloads
> > though... otherwise we have another weird flow of reading from the cache,
> > issuing multiple network requests and not writing the data to the cache. By
the
> > way, wasn't the long term plan to have the downloads code use range request
to
> > deal with partial downloads? That sounds cleaner.
> > > 
> > > The second caveat is that even if we ask for [last-], if we have a
significant
> > part of the resource, StopCaching can come before we finish serving data
from
> > the cache (aka, before we ask for [last-]). But I still don't want to be in
the
> > loop anymore :(
> > >
> > 
> > Understood. And I agree that we should try to get the cache transaction out
of
> > the loop as soon as possible.
> > 
> > However, we can't do so at the instant at which StopCaching() is called.
We'd
> > still need to crank the state machine until we get to a point where we can
> > guarantee that all the remaining bytes are going to come from the network.
> > 
> > How about this:
> >  - When StopCaching() is called, set up a flag without changing mode_.
> >  - At the completion of the next network read:
> >    - If caching is to be stopped.
> >      - If the network transaction is responsible for all remaining bytes:
[1]
> >        - Truncate cache entry or drop it as appropriate.
> >        - Release cache entry.
> >        - Switch to passthrough for remainder.
> >      - Else:
> >        - Skip writing, and continue with rest of state machine.
> > 
> > With the exception of [1], the rest is implemented in this CL.
> > 
> > I can also add the open ended range request logic which would mean that the
> > transaction will hit the condition in [1] sooner.
> > 
> > What do you think?
> >  
> > > Also, this bug report (crbug/347458) and patch
> > (https://codereview.chromium.org/181173003/) are also relevant for what
> > StopCaching should do.
> > 
> > Yup. I think [1] will address this.
> 
> That only partially addresses bug 347458 as it will keep the transaction
locked indefinitely (gated on the first download) while we wait to get to a
place when we can drop things.

Only until one read, unless it's a sparse entry.

> Counter proposal:
> 
> - Formalize that StopCaching behaves like other state changing methods of a
request, and cannot be called while Start or Read are in progress.

Hmm. I think this is doable. Let me take a whack at it.

> - When StopCaching is received abort any operations right away:
>   - If no ranges are involved || (truncated_ and reading after cached content)
:
>     - Fire and forget a write to truncate entry and release entry_
>   - if truncated_ and reading before cached content end || sparse_ (aka else)
>     - fire and forget write to truncate (if needed)
>     - release entry_
>     - delete network_transaction
>     - create network_transaction bytes:current-
>       - make sure to not return the headers again.

asanka

On 2015/07/17 at 00:08:33, asanka wrote: > On 2015/07/16 at 22:59:29, rvargas wrote: > > ...

5 years, 5 months ago (2015-07-17 00:40:43 UTC) #13

On 2015/07/17 at 00:08:33, asanka wrote:
> On 2015/07/16 at 22:59:29, rvargas wrote:
> > On 2015/07/16 21:05:23, asanka wrote:
> > > On 2015/07/16 at 19:22:56, rvargas wrote:
> > > > I've been looking at the bug report and I think that the bug is
somewhere
> > > else, with a caveat*.
> > > > 
> > > > The resource in question is not sparse, it is simply truncated, probably
by a
> > > previous download attempt.
> > > > 
> > > > In that case, we try to see if the server is ok with ranges, and then
ask for
> > > "the rest" of the request. The first issue that I see is that we are not
asking
> > > for a 64-bit range (or just [last-]) and instead we are truncating the
range end
> > > to 32-bits (GetNextRangeLen). Then the cache is "in the loop" for issuing
> > > another request after that :(.
> > > > 
> > > > We should change the code so that we either ask for 64 bit ranges, or
simply
> > > an open end request for truncated entries, so that we can get out of the
way
> > > later.
> > > >
> > > > 
> > > > *. The first caveat is of course that an entry could be sparse and not
just
> > > truncated. I still lean towards getting completely out of the way for
downloads
> > > though... otherwise we have another weird flow of reading from the cache,
> > > issuing multiple network requests and not writing the data to the cache.
By the
> > > way, wasn't the long term plan to have the downloads code use range
request to
> > > deal with partial downloads? That sounds cleaner.
> > > > 
> > > > The second caveat is that even if we ask for [last-], if we have a
significant
> > > part of the resource, StopCaching can come before we finish serving data
from
> > > the cache (aka, before we ask for [last-]). But I still don't want to be
in the
> > > loop anymore :(
> > > >
> > > 
> > > Understood. And I agree that we should try to get the cache transaction
out of
> > > the loop as soon as possible.
> > > 
> > > However, we can't do so at the instant at which StopCaching() is called.
We'd
> > > still need to crank the state machine until we get to a point where we can
> > > guarantee that all the remaining bytes are going to come from the network.
> > > 
> > > How about this:
> > >  - When StopCaching() is called, set up a flag without changing mode_.
> > >  - At the completion of the next network read:
> > >    - If caching is to be stopped.
> > >      - If the network transaction is responsible for all remaining bytes:
[1]
> > >        - Truncate cache entry or drop it as appropriate.
> > >        - Release cache entry.
> > >        - Switch to passthrough for remainder.
> > >      - Else:
> > >        - Skip writing, and continue with rest of state machine.
> > > 
> > > With the exception of [1], the rest is implemented in this CL.
> > > 
> > > I can also add the open ended range request logic which would mean that
the
> > > transaction will hit the condition in [1] sooner.
> > > 
> > > What do you think?
> > >  
> > > > Also, this bug report (crbug/347458) and patch
> > > (https://codereview.chromium.org/181173003/) are also relevant for what
> > > StopCaching should do.
> > > 
> > > Yup. I think [1] will address this.
> > 
> > That only partially addresses bug 347458 as it will keep the transaction
locked indefinitely (gated on the first download) while we wait to get to a
place when we can drop things.
> 
> Only until one read, unless it's a sparse entry.
> 
> > Counter proposal:
> > 
> > - Formalize that StopCaching behaves like other state changing methods of a
request, and cannot be called while Start or Read are in progress.
> 
> Hmm. I think this is doable. Let me take a whack at it.
> 
> > - When StopCaching is received abort any operations right away:
> >   - If no ranges are involved || (truncated_ and reading after cached
content) :
> >     - Fire and forget a write to truncate entry and release entry_
> >   - if truncated_ and reading before cached content end || sparse_ (aka
else)
> >     - fire and forget write to truncate (if needed)
> >     - release entry_
> >     - delete network_transaction
> >     - create network_transaction bytes:current-
> >       - make sure to not return the headers again.

A quick question before I do too much work on this: This is going to require
maching StopCaching() async since we'd need to prevent another Read() from being
issued while the network transaction is being restarted. We can accommodate an
async StopCaching() call without too much disruption to the ResourceLoader
stack. However, this does add a significant amount of complexity to the cache
transaction. Do you think that tradeoff is worth it? Note that we are only
preventing a delay in a questionable use case and even then in only a rare
subset of instances (i.e. downloading the same multiple times simultaneously
while said resource is associated with a sparse cache entry).

rvargas (doing something else)

On 2015/07/17 00:40:43, asanka wrote: > On 2015/07/17 at 00:08:33, asanka wrote: > > On ...

5 years, 5 months ago (2015-07-17 01:20:01 UTC) #14

On 2015/07/17 00:40:43, asanka wrote:
> On 2015/07/17 at 00:08:33, asanka wrote:
> > On 2015/07/16 at 22:59:29, rvargas wrote:
> > > On 2015/07/16 21:05:23, asanka wrote:
> > > > On 2015/07/16 at 19:22:56, rvargas wrote:
> > > > > I've been looking at the bug report and I think that the bug is
> somewhere
> > > > else, with a caveat*.
> > > > > 
> > > > > The resource in question is not sparse, it is simply truncated,
probably
> by a
> > > > previous download attempt.
> > > > > 
> > > > > In that case, we try to see if the server is ok with ranges, and then
> ask for
> > > > "the rest" of the request. The first issue that I see is that we are not
> asking
> > > > for a 64-bit range (or just [last-]) and instead we are truncating the
> range end
> > > > to 32-bits (GetNextRangeLen). Then the cache is "in the loop" for
issuing
> > > > another request after that :(.
> > > > > 
> > > > > We should change the code so that we either ask for 64 bit ranges, or
> simply
> > > > an open end request for truncated entries, so that we can get out of the
> way
> > > > later.
> > > > >
> > > > > 
> > > > > *. The first caveat is of course that an entry could be sparse and not
> just
> > > > truncated. I still lean towards getting completely out of the way for
> downloads
> > > > though... otherwise we have another weird flow of reading from the
cache,
> > > > issuing multiple network requests and not writing the data to the cache.
> By the
> > > > way, wasn't the long term plan to have the downloads code use range
> request to
> > > > deal with partial downloads? That sounds cleaner.
> > > > > 
> > > > > The second caveat is that even if we ask for [last-], if we have a
> significant
> > > > part of the resource, StopCaching can come before we finish serving data
> from
> > > > the cache (aka, before we ask for [last-]). But I still don't want to be
> in the
> > > > loop anymore :(
> > > > >
> > > > 
> > > > Understood. And I agree that we should try to get the cache transaction
> out of
> > > > the loop as soon as possible.
> > > > 
> > > > However, we can't do so at the instant at which StopCaching() is called.
> We'd
> > > > still need to crank the state machine until we get to a point where we
can
> > > > guarantee that all the remaining bytes are going to come from the
network.
> > > > 
> > > > How about this:
> > > >  - When StopCaching() is called, set up a flag without changing mode_.
> > > >  - At the completion of the next network read:
> > > >    - If caching is to be stopped.
> > > >      - If the network transaction is responsible for all remaining
bytes:
> [1]
> > > >        - Truncate cache entry or drop it as appropriate.
> > > >        - Release cache entry.
> > > >        - Switch to passthrough for remainder.
> > > >      - Else:
> > > >        - Skip writing, and continue with rest of state machine.
> > > > 
> > > > With the exception of [1], the rest is implemented in this CL.
> > > > 
> > > > I can also add the open ended range request logic which would mean that
> the
> > > > transaction will hit the condition in [1] sooner.
> > > > 
> > > > What do you think?
> > > >  
> > > > > Also, this bug report (crbug/347458) and patch
> > > > (https://codereview.chromium.org/181173003/) are also relevant for what
> > > > StopCaching should do.
> > > > 
> > > > Yup. I think [1] will address this.
> > > 
> > > That only partially addresses bug 347458 as it will keep the transaction
> locked indefinitely (gated on the first download) while we wait to get to a
> place when we can drop things.
> > 
> > Only until one read, unless it's a sparse entry.
> > 
> > > Counter proposal:
> > > 
> > > - Formalize that StopCaching behaves like other state changing methods of
a
> request, and cannot be called while Start or Read are in progress.
> > 
> > Hmm. I think this is doable. Let me take a whack at it.
> > 
> > > - When StopCaching is received abort any operations right away:
> > >   - If no ranges are involved || (truncated_ and reading after cached
> content) :
> > >     - Fire and forget a write to truncate entry and release entry_
> > >   - if truncated_ and reading before cached content end || sparse_ (aka
> else)
> > >     - fire and forget write to truncate (if needed)
> > >     - release entry_
> > >     - delete network_transaction
> > >     - create network_transaction bytes:current-
> > >       - make sure to not return the headers again.
> 
> A quick question before I do too much work on this: This is going to require
> maching StopCaching() async since we'd need to prevent another Read() from
being
> issued while the network transaction is being restarted. We can accommodate an
> async StopCaching() call without too much disruption to the ResourceLoader
> stack. However, this does add a significant amount of complexity to the cache
> transaction. Do you think that tradeoff is worth it? Note that we are only
> preventing a delay in a questionable use case and even then in only a rare
> subset of instances (i.e. downloading the same multiple times simultaneously
> while said resource is associated with a sparse cache entry).

From the bug conversation I was thinking about keeping the API synchronous.
That's why I was saying fire and forget write.

As for the next Read coming while we are starting then next transaction... I
haven't thought much about it. My idea was to remember what we were doing and
ignore the headers (as long as there is no error), but that would mean
remembering the Read arguments (not a big deal, but more messy).

You are right that making the API async is cleaner so if that's on the table it
sounds better.

As for what adds more complexity I'm not sure at this point. While the
problematic case is the inline download of a sparse entry (and I don't see how
that can happen in "normal" circumstances), I'm afraid we have to cover that
case unless we are sure it cannot happen. What are we doing today with save as
of a video?

<rant> what started as a simple optimization to not save a file twice at the
same time has grown many ugly legs and bugs over time </rant>

What happens if we make StopCaching fail if we reach the complex case? Could we
just restart the request from the filter bypassing the cache?

If we document that StopCaching cannot be called while Start or Read is in
progress (which doesn't require any codding), it seems that the only difference
between your proposal and mine is what to do in the complex case (sparse
entries, or truncated but still reading from the cache).

asanka

On 2015/07/17 at 01:20:01, rvargas wrote: > On 2015/07/17 00:40:43, asanka wrote: > > On ...

5 years, 5 months ago (2015-07-17 15:23:58 UTC) #15

On 2015/07/17 at 01:20:01, rvargas wrote:
> On 2015/07/17 00:40:43, asanka wrote:
> > On 2015/07/17 at 00:08:33, asanka wrote:
> > > On 2015/07/16 at 22:59:29, rvargas wrote:
> > > > On 2015/07/16 21:05:23, asanka wrote:
> > > > > On 2015/07/16 at 19:22:56, rvargas wrote:
> > > > > > I've been looking at the bug report and I think that the bug is
> > somewhere
> > > > > else, with a caveat*.
> > > > > > 
> > > > > > The resource in question is not sparse, it is simply truncated,
probably
> > by a
> > > > > previous download attempt.
> > > > > > 
> > > > > > In that case, we try to see if the server is ok with ranges, and
then
> > ask for
> > > > > "the rest" of the request. The first issue that I see is that we are
not
> > asking
> > > > > for a 64-bit range (or just [last-]) and instead we are truncating the
> > range end
> > > > > to 32-bits (GetNextRangeLen). Then the cache is "in the loop" for
issuing
> > > > > another request after that :(.
> > > > > > 
> > > > > > We should change the code so that we either ask for 64 bit ranges,
or
> > simply
> > > > > an open end request for truncated entries, so that we can get out of
the
> > way
> > > > > later.
> > > > > >
> > > > > > 
> > > > > > *. The first caveat is of course that an entry could be sparse and
not
> > just
> > > > > truncated. I still lean towards getting completely out of the way for
> > downloads
> > > > > though... otherwise we have another weird flow of reading from the
cache,
> > > > > issuing multiple network requests and not writing the data to the
cache.
> > By the
> > > > > way, wasn't the long term plan to have the downloads code use range
> > request to
> > > > > deal with partial downloads? That sounds cleaner.
> > > > > > 
> > > > > > The second caveat is that even if we ask for [last-], if we have a
> > significant
> > > > > part of the resource, StopCaching can come before we finish serving
data
> > from
> > > > > the cache (aka, before we ask for [last-]). But I still don't want to
be
> > in the
> > > > > loop anymore :(
> > > > > >
> > > > > 
> > > > > Understood. And I agree that we should try to get the cache
transaction
> > out of
> > > > > the loop as soon as possible.
> > > > > 
> > > > > However, we can't do so at the instant at which StopCaching() is
called.
> > We'd
> > > > > still need to crank the state machine until we get to a point where we
can
> > > > > guarantee that all the remaining bytes are going to come from the
network.
> > > > > 
> > > > > How about this:
> > > > >  - When StopCaching() is called, set up a flag without changing mode_.
> > > > >  - At the completion of the next network read:
> > > > >    - If caching is to be stopped.
> > > > >      - If the network transaction is responsible for all remaining
bytes:
> > [1]
> > > > >        - Truncate cache entry or drop it as appropriate.
> > > > >        - Release cache entry.
> > > > >        - Switch to passthrough for remainder.
> > > > >      - Else:
> > > > >        - Skip writing, and continue with rest of state machine.
> > > > > 
> > > > > With the exception of [1], the rest is implemented in this CL.
> > > > > 
> > > > > I can also add the open ended range request logic which would mean
that
> > the
> > > > > transaction will hit the condition in [1] sooner.
> > > > > 
> > > > > What do you think?
> > > > >  
> > > > > > Also, this bug report (crbug/347458) and patch
> > > > > (https://codereview.chromium.org/181173003/) are also relevant for
what
> > > > > StopCaching should do.
> > > > > 
> > > > > Yup. I think [1] will address this.
> > > > 
> > > > That only partially addresses bug 347458 as it will keep the transaction
> > locked indefinitely (gated on the first download) while we wait to get to a
> > place when we can drop things.
> > > 
> > > Only until one read, unless it's a sparse entry.
> > > 
> > > > Counter proposal:
> > > > 
> > > > - Formalize that StopCaching behaves like other state changing methods
of a
> > request, and cannot be called while Start or Read are in progress.
> > > 
> > > Hmm. I think this is doable. Let me take a whack at it.
> > > 
> > > > - When StopCaching is received abort any operations right away:
> > > >   - If no ranges are involved || (truncated_ and reading after cached
> > content) :
> > > >     - Fire and forget a write to truncate entry and release entry_
> > > >   - if truncated_ and reading before cached content end || sparse_ (aka
> > else)
> > > >     - fire and forget write to truncate (if needed)
> > > >     - release entry_
> > > >     - delete network_transaction
> > > >     - create network_transaction bytes:current-
> > > >       - make sure to not return the headers again.
> > 
> > A quick question before I do too much work on this: This is going to require
> > maching StopCaching() async since we'd need to prevent another Read() from
being
> > issued while the network transaction is being restarted. We can accommodate
an
> > async StopCaching() call without too much disruption to the ResourceLoader
> > stack. However, this does add a significant amount of complexity to the
cache
> > transaction. Do you think that tradeoff is worth it? Note that we are only
> > preventing a delay in a questionable use case and even then in only a rare
> > subset of instances (i.e. downloading the same multiple times simultaneously
> > while said resource is associated with a sparse cache entry).
> 
> From the bug conversation I was thinking about keeping the API synchronous.
That's why I was saying fire and forget write.
> 
> As for the next Read coming while we are starting then next transaction... I
haven't thought much about it. My idea was to remember what we were doing and
ignore the headers (as long as there is no error), but that would mean
remembering the Read arguments (not a big deal, but more messy).
> 
> You are right that making the API async is cleaner so if that's on the table
it sounds better.
> 
> As for what adds more complexity I'm not sure at this point. While the
problematic case is the inline download of a sparse entry (and I don't see how
that can happen in "normal" circumstances), I'm afraid we have to cover that
case unless we are sure it cannot happen. What are we doing today with save as
of a video?

A single inline download of a partially cached resource works fine currently
with the exception that the cache lock isn't released until the entire resource
is done with.

> <rant> what started as a simple optimization to not save a file twice at the
same time has grown many ugly legs and bugs over time </rant>

Hehe :) I'd however hasten to point out that this isn't merely an optimization.
Without it we'd have a much higher rate of download failures than the fairly
high rate of disk full errors we are seeing as it is. Even without cache
eviction concerns, writing two copies of the same potentially large resource on
disk is also pretty bad for resource constrained environments like netbooks. But
you know this better than I do.

> What happens if we make StopCaching fail if we reach the complex case? Could
we just restart the request from the filter bypassing the cache?

We could, but I think we'd end up back in the situation where StopCaching()
followed by a Read() will need to coordinate across a network transaction
restart. Or am I misunderstanding your suggestion?

> If we document that StopCaching cannot be called while Start or Read is in
progress (which doesn't require any codding), it seems that the only difference
between your proposal and mine is what to do in the complex case (sparse
entries, or truncated but still reading from the cache).

Agreed.

rvargas (doing something else)

On 2015/07/17 15:23:58, asanka wrote: > On 2015/07/17 at 01:20:01, rvargas wrote: > > On ...

5 years, 5 months ago (2015-07-17 18:05:50 UTC) #16

On 2015/07/17 15:23:58, asanka wrote:
> On 2015/07/17 at 01:20:01, rvargas wrote:
> > On 2015/07/17 00:40:43, asanka wrote:
> > > On 2015/07/17 at 00:08:33, asanka wrote:
> > > > On 2015/07/16 at 22:59:29, rvargas wrote:
> > > > > On 2015/07/16 21:05:23, asanka wrote:
> > > > > > On 2015/07/16 at 19:22:56, rvargas wrote:
> > > > > > > I've been looking at the bug report and I think that the bug is
> > > somewhere
> > > > > > else, with a caveat*.
> > > > > > > 
> > > > > > > The resource in question is not sparse, it is simply truncated,
> probably
> > > by a
> > > > > > previous download attempt.
> > > > > > > 
> > > > > > > In that case, we try to see if the server is ok with ranges, and
> then
> > > ask for
> > > > > > "the rest" of the request. The first issue that I see is that we are
> not
> > > asking
> > > > > > for a 64-bit range (or just [last-]) and instead we are truncating
the
> > > range end
> > > > > > to 32-bits (GetNextRangeLen). Then the cache is "in the loop" for
> issuing
> > > > > > another request after that :(.
> > > > > > > 
> > > > > > > We should change the code so that we either ask for 64 bit ranges,
> or
> > > simply
> > > > > > an open end request for truncated entries, so that we can get out of
> the
> > > way
> > > > > > later.
> > > > > > >
> > > > > > > 
> > > > > > > *. The first caveat is of course that an entry could be sparse and
> not
> > > just
> > > > > > truncated. I still lean towards getting completely out of the way
for
> > > downloads
> > > > > > though... otherwise we have another weird flow of reading from the
> cache,
> > > > > > issuing multiple network requests and not writing the data to the
> cache.
> > > By the
> > > > > > way, wasn't the long term plan to have the downloads code use range
> > > request to
> > > > > > deal with partial downloads? That sounds cleaner.
> > > > > > > 
> > > > > > > The second caveat is that even if we ask for [last-], if we have a
> > > significant
> > > > > > part of the resource, StopCaching can come before we finish serving
> data
> > > from
> > > > > > the cache (aka, before we ask for [last-]). But I still don't want
to
> be
> > > in the
> > > > > > loop anymore :(
> > > > > > >
> > > > > > 
> > > > > > Understood. And I agree that we should try to get the cache
> transaction
> > > out of
> > > > > > the loop as soon as possible.
> > > > > > 
> > > > > > However, we can't do so at the instant at which StopCaching() is
> called.
> > > We'd
> > > > > > still need to crank the state machine until we get to a point where
we
> can
> > > > > > guarantee that all the remaining bytes are going to come from the
> network.
> > > > > > 
> > > > > > How about this:
> > > > > >  - When StopCaching() is called, set up a flag without changing
mode_.
> > > > > >  - At the completion of the next network read:
> > > > > >    - If caching is to be stopped.
> > > > > >      - If the network transaction is responsible for all remaining
> bytes:
> > > [1]
> > > > > >        - Truncate cache entry or drop it as appropriate.
> > > > > >        - Release cache entry.
> > > > > >        - Switch to passthrough for remainder.
> > > > > >      - Else:
> > > > > >        - Skip writing, and continue with rest of state machine.
> > > > > > 
> > > > > > With the exception of [1], the rest is implemented in this CL.
> > > > > > 
> > > > > > I can also add the open ended range request logic which would mean
> that
> > > the
> > > > > > transaction will hit the condition in [1] sooner.
> > > > > > 
> > > > > > What do you think?
> > > > > >  
> > > > > > > Also, this bug report (crbug/347458) and patch
> > > > > > (https://codereview.chromium.org/181173003/) are also relevant for
> what
> > > > > > StopCaching should do.
> > > > > > 
> > > > > > Yup. I think [1] will address this.
> > > > > 
> > > > > That only partially addresses bug 347458 as it will keep the
transaction
> > > locked indefinitely (gated on the first download) while we wait to get to
a
> > > place when we can drop things.
> > > > 
> > > > Only until one read, unless it's a sparse entry.
> > > > 
> > > > > Counter proposal:
> > > > > 
> > > > > - Formalize that StopCaching behaves like other state changing methods
> of a
> > > request, and cannot be called while Start or Read are in progress.
> > > > 
> > > > Hmm. I think this is doable. Let me take a whack at it.
> > > > 
> > > > > - When StopCaching is received abort any operations right away:
> > > > >   - If no ranges are involved || (truncated_ and reading after cached
> > > content) :
> > > > >     - Fire and forget a write to truncate entry and release entry_
> > > > >   - if truncated_ and reading before cached content end || sparse_
(aka
> > > else)
> > > > >     - fire and forget write to truncate (if needed)
> > > > >     - release entry_
> > > > >     - delete network_transaction
> > > > >     - create network_transaction bytes:current-
> > > > >       - make sure to not return the headers again.
> > > 
> > > A quick question before I do too much work on this: This is going to
require
> > > maching StopCaching() async since we'd need to prevent another Read() from
> being
> > > issued while the network transaction is being restarted. We can
accommodate
> an
> > > async StopCaching() call without too much disruption to the ResourceLoader
> > > stack. However, this does add a significant amount of complexity to the
> cache
> > > transaction. Do you think that tradeoff is worth it? Note that we are only
> > > preventing a delay in a questionable use case and even then in only a rare
> > > subset of instances (i.e. downloading the same multiple times
simultaneously
> > > while said resource is associated with a sparse cache entry).
> > 
> > From the bug conversation I was thinking about keeping the API synchronous.
> That's why I was saying fire and forget write.
> > 
> > As for the next Read coming while we are starting then next transaction... I
> haven't thought much about it. My idea was to remember what we were doing and
> ignore the headers (as long as there is no error), but that would mean
> remembering the Read arguments (not a big deal, but more messy).
> > 
> > You are right that making the API async is cleaner so if that's on the table
> it sounds better.
> > 
> > As for what adds more complexity I'm not sure at this point. While the
> problematic case is the inline download of a sparse entry (and I don't see how
> that can happen in "normal" circumstances), I'm afraid we have to cover that
> case unless we are sure it cannot happen. What are we doing today with save as
> of a video?
> 
> A single inline download of a partially cached resource works fine currently
> with the exception that the cache lock isn't released until the entire
resource
> is done with.

you mean it would work after this CL, not on ToT, right?.

> 
> > <rant> what started as a simple optimization to not save a file twice at the
> same time has grown many ugly legs and bugs over time </rant>
> 
> Hehe :) I'd however hasten to point out that this isn't merely an
optimization.
> Without it we'd have a much higher rate of download failures than the fairly
> high rate of disk full errors we are seeing as it is. Even without cache
> eviction concerns, writing two copies of the same potentially large resource
on
> disk is also pretty bad for resource constrained environments like netbooks.
But
> you know this better than I do.
> 
> > What happens if we make StopCaching fail if we reach the complex case? Could
> we just restart the request from the filter bypassing the cache?
> 
> We could, but I think we'd end up back in the situation where StopCaching()
> followed by a Read() will need to coordinate across a network transaction
> restart. Or am I misunderstanding your suggestion?

I'm not sure. What I meant is to have a return value on StopCaching so that
whenever we see one of the two problematic conditions we return failure. The
downloads handler will then see that result and instead of keep reading, would
cancel the request and issue a new one with a flag to bypass the cache. (the
render is not involved anymore) The new URLRequest should go out right away
because the first request is gone.
 

> 
> > If we document that StopCaching cannot be called while Start or Read is in
> progress (which doesn't require any codding), it seems that the only
difference
> between your proposal and mine is what to do in the complex case (sparse
> entries, or truncated but still reading from the cache).
> 
> Agreed.

asanka

On 2015/07/17 at 18:05:50, rvargas wrote: > On 2015/07/17 15:23:58, asanka wrote: > > On ...

5 years, 5 months ago (2015-07-17 18:58:51 UTC) #17

On 2015/07/17 at 18:05:50, rvargas wrote:
> On 2015/07/17 15:23:58, asanka wrote:
> > On 2015/07/17 at 01:20:01, rvargas wrote:
> > > On 2015/07/17 00:40:43, asanka wrote:
> > > > On 2015/07/17 at 00:08:33, asanka wrote:
> > > > > On 2015/07/16 at 22:59:29, rvargas wrote:
> > > > > > On 2015/07/16 21:05:23, asanka wrote:
> > > > > > > On 2015/07/16 at 19:22:56, rvargas wrote:
> > > > > > > > I've been looking at the bug report and I think that the bug is
> > > > somewhere
> > > > > > > else, with a caveat*.
> > > > > > > > 
> > > > > > > > The resource in question is not sparse, it is simply truncated,
> > probably
> > > > by a
> > > > > > > previous download attempt.
> > > > > > > > 
> > > > > > > > In that case, we try to see if the server is ok with ranges, and
> > then
> > > > ask for
> > > > > > > "the rest" of the request. The first issue that I see is that we
are
> > not
> > > > asking
> > > > > > > for a 64-bit range (or just [last-]) and instead we are truncating
the
> > > > range end
> > > > > > > to 32-bits (GetNextRangeLen). Then the cache is "in the loop" for
> > issuing
> > > > > > > another request after that :(.
> > > > > > > > 
> > > > > > > > We should change the code so that we either ask for 64 bit
ranges,
> > or
> > > > simply
> > > > > > > an open end request for truncated entries, so that we can get out
of
> > the
> > > > way
> > > > > > > later.
> > > > > > > >
> > > > > > > > 
> > > > > > > > *. The first caveat is of course that an entry could be sparse
and
> > not
> > > > just
> > > > > > > truncated. I still lean towards getting completely out of the way
for
> > > > downloads
> > > > > > > though... otherwise we have another weird flow of reading from the
> > cache,
> > > > > > > issuing multiple network requests and not writing the data to the
> > cache.
> > > > By the
> > > > > > > way, wasn't the long term plan to have the downloads code use
range
> > > > request to
> > > > > > > deal with partial downloads? That sounds cleaner.
> > > > > > > > 
> > > > > > > > The second caveat is that even if we ask for [last-], if we have
a
> > > > significant
> > > > > > > part of the resource, StopCaching can come before we finish
serving
> > data
> > > > from
> > > > > > > the cache (aka, before we ask for [last-]). But I still don't want
to
> > be
> > > > in the
> > > > > > > loop anymore :(
> > > > > > > >
> > > > > > > 
> > > > > > > Understood. And I agree that we should try to get the cache
> > transaction
> > > > out of
> > > > > > > the loop as soon as possible.
> > > > > > > 
> > > > > > > However, we can't do so at the instant at which StopCaching() is
> > called.
> > > > We'd
> > > > > > > still need to crank the state machine until we get to a point
where we
> > can
> > > > > > > guarantee that all the remaining bytes are going to come from the
> > network.
> > > > > > > 
> > > > > > > How about this:
> > > > > > >  - When StopCaching() is called, set up a flag without changing
mode_.
> > > > > > >  - At the completion of the next network read:
> > > > > > >    - If caching is to be stopped.
> > > > > > >      - If the network transaction is responsible for all remaining
> > bytes:
> > > > [1]
> > > > > > >        - Truncate cache entry or drop it as appropriate.
> > > > > > >        - Release cache entry.
> > > > > > >        - Switch to passthrough for remainder.
> > > > > > >      - Else:
> > > > > > >        - Skip writing, and continue with rest of state machine.
> > > > > > > 
> > > > > > > With the exception of [1], the rest is implemented in this CL.
> > > > > > > 
> > > > > > > I can also add the open ended range request logic which would mean
> > that
> > > > the
> > > > > > > transaction will hit the condition in [1] sooner.
> > > > > > > 
> > > > > > > What do you think?
> > > > > > >  
> > > > > > > > Also, this bug report (crbug/347458) and patch
> > > > > > > (https://codereview.chromium.org/181173003/) are also relevant for
> > what
> > > > > > > StopCaching should do.
> > > > > > > 
> > > > > > > Yup. I think [1] will address this.
> > > > > > 
> > > > > > That only partially addresses bug 347458 as it will keep the
transaction
> > > > locked indefinitely (gated on the first download) while we wait to get
to a
> > > > place when we can drop things.
> > > > > 
> > > > > Only until one read, unless it's a sparse entry.
> > > > > 
> > > > > > Counter proposal:
> > > > > > 
> > > > > > - Formalize that StopCaching behaves like other state changing
methods
> > of a
> > > > request, and cannot be called while Start or Read are in progress.
> > > > > 
> > > > > Hmm. I think this is doable. Let me take a whack at it.
> > > > > 
> > > > > > - When StopCaching is received abort any operations right away:
> > > > > >   - If no ranges are involved || (truncated_ and reading after
cached
> > > > content) :
> > > > > >     - Fire and forget a write to truncate entry and release entry_
> > > > > >   - if truncated_ and reading before cached content end || sparse_
(aka
> > > > else)
> > > > > >     - fire and forget write to truncate (if needed)
> > > > > >     - release entry_
> > > > > >     - delete network_transaction
> > > > > >     - create network_transaction bytes:current-
> > > > > >       - make sure to not return the headers again.
> > > > 
> > > > A quick question before I do too much work on this: This is going to
require
> > > > maching StopCaching() async since we'd need to prevent another Read()
from
> > being
> > > > issued while the network transaction is being restarted. We can
accommodate
> > an
> > > > async StopCaching() call without too much disruption to the
ResourceLoader
> > > > stack. However, this does add a significant amount of complexity to the
> > cache
> > > > transaction. Do you think that tradeoff is worth it? Note that we are
only
> > > > preventing a delay in a questionable use case and even then in only a
rare
> > > > subset of instances (i.e. downloading the same multiple times
simultaneously
> > > > while said resource is associated with a sparse cache entry).
> > > 
> > > From the bug conversation I was thinking about keeping the API
synchronous.
> > That's why I was saying fire and forget write.
> > > 
> > > As for the next Read coming while we are starting then next transaction...
I
> > haven't thought much about it. My idea was to remember what we were doing
and
> > ignore the headers (as long as there is no error), but that would mean
> > remembering the Read arguments (not a big deal, but more messy).
> > > 
> > > You are right that making the API async is cleaner so if that's on the
table
> > it sounds better.
> > > 
> > > As for what adds more complexity I'm not sure at this point. While the
> > problematic case is the inline download of a sparse entry (and I don't see
how
> > that can happen in "normal" circumstances), I'm afraid we have to cover that
> > case unless we are sure it cannot happen. What are we doing today with save
as
> > of a video?
> > 
> > A single inline download of a partially cached resource works fine currently
> > with the exception that the cache lock isn't released until the entire
resource
> > is done with.
> 
> you mean it would work after this CL, not on ToT, right?.

Right. Sorry about not being clear.

Sparse entries don't work reliably on ToT. If StopCaching() is called during a
partial network read, the transaction switches to pass-through mode and signals
EOF when the network stream ends.

> > > <rant> what started as a simple optimization to not save a file twice at
the
> > same time has grown many ugly legs and bugs over time </rant>
> > 
> > Hehe :) I'd however hasten to point out that this isn't merely an
optimization.
> > Without it we'd have a much higher rate of download failures than the fairly
> > high rate of disk full errors we are seeing as it is. Even without cache
> > eviction concerns, writing two copies of the same potentially large resource
on
> > disk is also pretty bad for resource constrained environments like netbooks.
But
> > you know this better than I do.
> > 
> > > What happens if we make StopCaching fail if we reach the complex case?
Could
> > we just restart the request from the filter bypassing the cache?
> > 
> > We could, but I think we'd end up back in the situation where StopCaching()
> > followed by a Read() will need to coordinate across a network transaction
> > restart. Or am I misunderstanding your suggestion?
> 
> I'm not sure. What I meant is to have a return value on StopCaching so that
whenever we see one of the two problematic conditions we return failure. The
downloads handler will then see that result and instead of keep reading, would
cancel the request and issue a new one with a flag to bypass the cache. (the
render is not involved anymore) The new URLRequest should go out right away
because the first request is gone.
>  

Ah. This would work hopefully in the near future. Having downloads initiate
requests in the absence of or impersonating a renderer is currently fraught with
peril. The ResourceLoader stack which drives these URLRequests currently
strongly bound to a renderer. The work proposed is to disentangle these
dependencies.

> > > If we document that StopCaching cannot be called while Start or Read is in
> > progress (which doesn't require any codding), it seems that the only
difference
> > between your proposal and mine is what to do in the complex case (sparse
> > entries, or truncated but still reading from the cache).
> > 
> > Agreed.

asanka

rvargas@ and I had an OOB conversation. As promised there, here's a brief overview of ...

5 years, 5 months ago (2015-07-20 23:18:58 UTC) #18

rvargas@ and I had an OOB conversation. As promised there, here's a brief
overview of the different approaches we have in front of us:

A. StopCaching() kills all cache operations.

  1. If the entry is being updated or written, then the cache entry should be
     flagged as truncated or doomed depending on whether the request can be
     resumed.

    1.1 No other pending operations can be in progress when StopCaching() is
        called.

    1.2 StopCaching() must be asynchronous so that the caller knows when
        it is safe to issue another Read().

        The only effective caller of StopCaching() is DownloadResourceHandler
        which does so while processing the OnResponseStarted() event in the
        ResourceLoader stack. This replays the eponymous URLRequest::Delegate
        event.

        DownloadResourceHandler will need to do one of the following (not
        exhaustive):

        a) defer OnResponseStarted(), or

        b) indicate to the ResorceLoader that it has called StopCaching() so
that
           the ResourceLoader itself can wait for a signal from the URLRequest()
           that the StopCaching() operation is complete or

        c) invoke a new method on the ResourceLoader requesting that the
           ResourceLoader invoke StopCaching() on the URLRequest and wait for
its
           completion prior to issuing any new Read()s.

        Currently all asynchronous operation callbacks from URLRequests are done
        via its Delegate interface. Hence, for consistency, the completion of an
        async StopCaching() would also need to follow suit. Since ResourceLoader
        is the URLRequest::Delegate for loader owned requests, option c) above
        is the most consistent with existing code.

  2. If there's an active network transaction and it has not been
     conditionalized or if the request is for the last range, then the cache
     transaction can switch to pass through mode. All subsequent Read()s will be
     redirected to the network transaction.

  3. If #2 is false, then a new request needs to be synthesized for the
     remainder of the originally requested resource.

     Note: It is possible for the request to not be resumable (i.e. the request
     cannot be conditionalized). This can happen if the entire resource is in
     the cache and it is currently being served out of cache. In this case the
     HttpCache::Transaction needs to keep serving the remainder of the resource
     out of cache. This is also the case for requests that bypass the cache.

     Synthesizing a new request requires:

     3.1 StopCaching() be issued while there are no pending operations, and that
         it be asynchronous. Similar to 1.1 and 1.2 above.

     3.2 Absorbing the headers of the newly synthesized request and verifying
         that the server has responded with a valid partial 206 response. Any
         other response results in an error.

B. StopCaching() returns an error if the transaction can't switch to passthrough
   mode immediately.

   1. If there's an active network transaction that satisfies A.2 above, then
      the transaction can switch to passthrough mode synchronously (assuming
      A.1.1 is met).

   2. If #1 is false, StopCaching() returns an error signalling to the caller
      that the transaction needs to be restarted.
   
      Since the response body may have been partially read already, the
      transaction needs to be restated by the caller who also needs to deal with
      discarding any data that has already been read, or issuing a
      conditionalized request for the remainder of the resource.

      The caller, in this case is whomever has consumed all the bytes up to the
      point StopCaching() was called. Since currently the only caller of
      StopCaching() is DownloadResourceHandler, this would only be the downloads
      subsystem. Any new callers of StopCaching() would need to account for the
      possibility that the request could terminate.

      DownloadResourceHandler is owned by the ResourceLoader who is lifetime
      bound with the URLRequest. Hence, cancellation of the request, by design,
      causes the ResourceLoader and the ResourceHandler chain to be deleted.
      Hence, responsibility for restarting the request lies with a higher level
      caller.  This will be the DownloadManager.

      2.1 DownloadResourceHandler invokes DownloadManager::StartDownload() upon
          receipt of a OnResponseStarted notification. This is  done regardless
          of the outcome of the StopCaching().

      2.2 If the StopCaching() call fails, this failure is reported to the
          DownloadManager as a download interruption. The DownloadManager then
          cancels the request (thereby tearing down the URLRequest and in turn
          the HttpCache::Transaction). This notification must be issued prior to
          the DownloadItem starts filename determination.

      2.3 DownloadItem handles the interruption due to the StopCaching() failure
          by immediately calling Resume() on itself. This results in a new
          URLRequest to be created and handed over to the
          ResourceDispatcherHost. This request is expected to proceed without
          (much) delay since the cache lock has already been relinquished by 2.2
          above.

          Requests created here will need to specify the LOAD_BYPASS_CACHE flag.

          Note: Handling of requests created under non-default StoragePartitions
          for a profile aren't correctly handled currently.

      Requests that can't be restarted (due to not being idempotent, for
      example) cannot be handled in this manner. However, such requests are
      expected to fulfil the conditions in A.2.

C. (This CL) StopCaching() synchronously sets a flag, but doesn't affect the
state
   machine. The flag is picked up by the next Read() cycle in order to
   asynchronously release the cache lock at the earliest opportunity.

   The external API remains the same. The cache lock remains in effect until the
   next Read() operation satisfies the requirement in A.2. At that point, the
   cache lock can be relinquished as described in A.1.

rvargas (doing something else)

I still think we should change the API to specify that StopCaching should not be ...

5 years, 4 months ago (2015-07-27 22:25:12 UTC) #19

asanka

Sorry about the delay. Got distracted by a few things. I've updated the description to ...

5 years, 4 months ago (2015-08-18 22:46:59 UTC) #20

rvargas (doing something else)

Thanks. https://codereview.chromium.org/1230113012/diff/80001/net/http/partial_data.cc File net/http/partial_data.cc (right): https://codereview.chromium.org/1230113012/diff/80001/net/http/partial_data.cc#newcode444 net/http/partial_data.cc:444: return byte_range_.HasLastBytePosition() On 2015/08/18 22:46:59, asanka wrote: > ...

5 years, 4 months ago (2015-08-19 23:46:39 UTC) #21

asanka

PTAL? This simplifies the HttpCache <-> HttpCache::Transaction interaction a bit by making it the Cache's ...

5 years, 3 months ago (2015-09-04 19:09:04 UTC) #22

PTAL? This simplifies the HttpCache <-> HttpCache::Transaction interaction a bit
by making it the Cache's responsibility to know how to release a cache entry.
Also addressing some sate machine mishandling in the previous patchset.

https://codereview.chromium.org/1230113012/diff/80001/net/http/partial_data.cc
File net/http/partial_data.cc (right):

https://codereview.chromium.org/1230113012/diff/80001/net/http/partial_data.c...
net/http/partial_data.cc:444: return byte_range_.HasLastBytePosition()
On 2015/08/19 at 23:46:37, rvargas (out until 9-8) wrote:
> On 2015/08/18 22:46:59, asanka wrote:
> > On 2015/07/27 at 22:25:12, rvargas wrote:
> > > It seems like we should eliminate this method.
> > 
> > New patchset adds another call site. Let me know if you still this we should
get
> > rid of it.
> 
> The reason was that the code at the call sites would be clearer if this is
inlined+simplified.
> 
> That may not be true for the new call site, but at the same time, what does it
mean to call SkipCacheForRemainder if !byte_range_.HasLastBytePos()?
> 
> I think I still lean to get rid of this method even if we add a little code
duplication.

SG. Removed. I think it also makes the handling of the kUnbounded case much more
explicit.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
File net/http/http_cache_transaction.cc (left):

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:532: if (cache_.get() && entry_) {
On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> We still need this code, otherwise it will crash at cache_->

Done.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:537: // It is necessary to check mode_ & READ
because it is possible
On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> We should be able to fix this code now and make this a plain else. (or even
better do a single call to the cache)

Done.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1781: // If there is an error or we aren't
saving the data, we are done; just wait
On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> Why remove this?

Collateral. In an intermediate stage I experimented with explicitly removing the
cache lock at this point.

Comment restored.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:2707: is_sparse_ = false;
On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> We're losing this part (and the transaction will be basically restarted).

Reinstated.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:460: if (stopped_caching_ && (mode_ & WRITE)
&&
On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> Why do we need to check the mode here? (how can we drop that bit from 
StopCaching without dropping entry_?)

I removed the mode_ check from StopCaching() and we are checking it here
instead. StopCaching() could be called before Start() is called which could
result in mode_ not being set by the time StopCaching() is called.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:461: !(effective_load_flags_ &
LOAD_PREFERRING_CACHE) && entry_) {
On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> why do we look at load_flags_?

Originally did so because LOAD_PREFERRING_CACHE may have caused us to skip
validation. But it's also the case that once we've decided to use the cached
entry we also drop the WRITE bit. Removed the check.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:482: DCHECK(!(effective_load_flags_ &
LOAD_PREFETCH));
On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> isn't this outside of the control of this code? What would break in the cache
if this is violated by callers?

Not sure how this crept in. Removed.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:487: // exclusively from the cache already,
then we are going to ignore the
On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> I'd probably remove the second sentence, as it is a little confusing because
it talks about the opposite logic of what is tested by the code. The first
sentence already says why we only care for writes.

Removed the check and comment since we just set the flag here.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1394: mode_ = stopped_caching_ ? NONE :
WRITE;
On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> If we switch to NONE, shouldn't we release the cache entry?

Removed this logic.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1458: if (reading_) {
On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> Is this block related to this change? can we make it into a dedicated cl?

Removed.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1483: // the network.
On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> why?. Or what do you mean by "exclusively". This may be just another range in
the middle.
> 
> Why don't we let this go until next Read() ?

I expanded the condition here to only include the case where it makes sense to
abandon the cache entry and continue using the network. Look again?

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1905: if (entry_) {
On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> Why can we be here without entry_?

OnWriteResponseInfoToEntry() will doom and release the entry if there's an error
writing the response info.

(I moved this logic to AbandonCacheEntry() and friends.)

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1916: if (request_->method != "GET") {
On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> look at this on StopCaching?

StopCaching() can be called prior to Start() being called.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1927: // Unless this was a sparse entry or a
complete preexiting entry, just get
On 2015/08/19 at 23:46:39, rvargas (out until 9-8) wrote:
> typo: preexisting

comment changed.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1933: mode_ = NONE;
On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> Do this inside Realease?

Done.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:2214: DCHECK(!reading_);  //
Conditionalization should only depend on the state
On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> nit:
> 
> // comment
> // blah
> DCHECK();

Done.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:2748: // entirety prior to this transaction.
On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> Why in its entirety?. If there's a truncated entry and we have a request for
it, and the caller decided that as far as it is concerned the entry can be gone,
doesn't the fact that the entry was already there isn't enough to not delete it?

The deletion is based on the assumption that most of the truncated cache entries
whom StopCaching() is called on are stubs left over from previous download
requests. I.e. the stubs weren't helping anyone by existing.

That said, I don't have any numbers to back up this claim, so I'm willing to
leave preexisting stubs be if you think we should leave them.

(The code here has changed and doesn't exist anymore. But the logic still
survives in AbandonCacheEntry() and friends).

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:2756: if (!entry_->writer) {
On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> We should let the cache decide this part.

Done.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:2767: entry_disposition = CACHE_ENTRY_DOOM;
On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> Does this do anything?

DoneWithEntry would only destroy the entry and fire off ERR_CACHE_RACE errors to
pending transactions if this was set. Moved to HttpCache::DoneWithEntry()
instead.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:2776: return DoCacheWriteTruncatedResponse();
On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> I don't believe we call a SM method directly anywhere... they are only called
from within the SM switch as part of a formal state transition.
> 
> Can we separate what is called from within DoLoop and what is not? The fact
that this method is called both from within and from outside the SM makes this
attempt to re-enter the SM suspicious.

Yeah. Fixed. I moved the logic for abandoning a cache entry out of the main SM
since it needs to be invoked outside of the main SM flow.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:2959: if (transaction_pattern_ ==
PATTERN_UNDEFINED)
On 2015/08/19 at 23:46:39, rvargas (out until 9-8) wrote:
> Now that we can reach this code without having inspected the operation enough
to know the pattern, we should call this method exclusively from the destructor.

Done. Also consolidated with next line.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
File net/http/http_cache_transaction.h (right):

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.h:213: // These states are entered from Read
On 2015/08/19 at 23:46:39, rvargas (out until 9-8) wrote:
> nit: period at the end.

Done.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.h:295: int
DoReleaseCacheEntryAndContinueUsingNetwork();
On 2015/08/19 at 23:46:39, rvargas (out until 9-8) wrote:
> nit: keep the same order of the enum.

Done.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.h:411: //   Marks the cache entry as doomed.
On 2015/08/19 at 23:46:39, rvargas (out until 9-8) wrote:
> nit: Dooms the entry (there's no really a doom mark).

Done.

https://codereview.chromium.org/1230113012/diff/160001/net/http/partial_data.cc
File net/http/partial_data.cc (right):

https://codereview.chromium.org/1230113012/diff/160001/net/http/partial_data....
net/http/partial_data.cc:29: const int64 kUnbounded =
std::numeric_limits<int64>::max();
On 2015/08/19 at 23:46:39, rvargas (out until 9-8) wrote:
> A recent chr-dev thread suggest this may use an static initializer with some
compilers. Maybe use stdint?

Done. I used basictypes.h instead of sttdint because we are still using int64
instead of int64_t.

https://codereview.chromium.org/1230113012/diff/160001/net/http/partial_data....
net/http/partial_data.cc:127: } else if (!truncated_) {
On 2015/08/19 at 23:46:39, rvargas (out until 9-8) wrote:
> Aren't we going through this else after SkipCacheForRemainder? If so, do we
care about the value of truncated_?

Changed SkipCacheForRemainder() to explicitly set truncated_ to true. In
practice we are not calling SkipCacheForRemainder() for the !sparse &&
!truncated case. The latter implies that the entire resource is cached and we
don't honor StopCaching() for that case. However that logic is outside of
PartialData.

https://codereview.chromium.org/1230113012/diff/160001/net/http/partial_data.h
File net/http/partial_data.h (right):

https://codereview.chromium.org/1230113012/diff/160001/net/http/partial_data....
net/http/partial_data.h:81: // Switch to network reads for the remainder of the
request.
On 2015/08/19 at 23:46:39, rvargas (out until 9-8) wrote:
> Document what is the return value (the caller is assuming some specific
behavior here)

Done.

rvargas (doing something else)

Sorry for the delay. Almost there. https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_transaction.cc#newcode460 net/http/http_cache_transaction.cc:460: if (stopped_caching_ && ...

5 years, 3 months ago (2015-09-11 23:56:18 UTC) #23

hubbe

hubbe@chromium.org changed reviewers: - hubbe@chromium.org

5 years, 3 months ago (2015-09-14 19:13:29 UTC) #24

rvargas (doing something else)

I haven't looked at the tests in detail. https://codereview.chromium.org/1230113012/diff/220001/net/http/disk_based_cert_cache_unittest.cc File net/http/disk_based_cert_cache_unittest.cc (right): https://codereview.chromium.org/1230113012/diff/220001/net/http/disk_based_cert_cache_unittest.cc#newcode48 net/http/disk_based_cert_cache_unittest.cc:48: key, ...

5 years, 3 months ago (2015-09-15 01:07:39 UTC) #25

asanka

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_transaction.cc#newcode1889 net/http/http_cache_transaction.cc:1889: // have to keep the entry around to be ...

5 years, 3 months ago (2015-09-17 21:59:41 UTC) #26

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1889: // have to keep the entry around to be
flagged as truncated later on.
On 2015/08/19 at 23:46:39, rvargas (slow to respond) wrote:
> nit: "End of file" is not a given anymore (as it is in the next if); the same
goes for "this may be the result...". Maybe Remove all that and say "see if we
have...", or rewrite to make clear that of or error is part of whet we have to
consider. (yes, this comment/if condition is far from clear)

Comment expanded.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1916: if (request_->method != "GET") {
On 2015/09/11 at 23:56:17, rvargas (slow to respond) wrote:
> On 2015/09/04 19:09:03, asanka wrote:
> > On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> > > look at this on StopCaching?
> > 
> > StopCaching() can be called prior to Start() being called.
> 
> Wait, why do we want to support that? That use case would be equivalent to a
load flag combination.

It was done initially on the assumption that callers of StopCaching() may not
necessarily be in a position to set load flags. But in retrospect that was
unnecessary, and certainly not something the network stack should be concerned
with.

I reworked the logic to not support calling StopCaching() before Start(). Added
a DCHECK() in StopCaching() and also moved the verb and WRITE bit checks there.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1934: stopped_caching_ = false;
On 2015/09/11 at 23:56:17, rvargas (slow to respond) wrote:
> On 2015/08/19 23:46:38, rvargas (slow to respond) wrote:
> > Do we have to set this to false (and elsewhere)? If it doesn't matter (or
it's
> > easy to accommodate), I think it is better to leave it on as it will show up
in
> > the occasional crash report and it's good to know what we did to this entry.
> 
> I think you missed this :)

Indeed! Done.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:2748: // entirety prior to this transaction.
On 2015/09/11 at 23:56:17, rvargas (slow to respond) wrote:
> On 2015/09/04 19:09:03, asanka wrote:
> > On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> > > Why in its entirety?. If there's a truncated entry and we have a request
for
> > it, and the caller decided that as far as it is concerned the entry can be
gone,
> > doesn't the fact that the entry was already there isn't enough to not delete
it?
> > 
> > The deletion is based on the assumption that most of the truncated cache
entries
> > whom StopCaching() is called on are stubs left over from previous download
> > requests. I.e. the stubs weren't helping anyone by existing.
> > 
> > That said, I don't have any numbers to back up this claim, so I'm willing to
> > leave preexisting stubs be if you think we should leave them.
> > 
> > (The code here has changed and doesn't exist anymore. But the logic still
> > survives in AbandonCacheEntry() and friends).
> 
> I suspect the assumption is correct, but I also would not be surprised if some
truncated entries are not the result of a failed download and then there's a
download request[*].
> 
> On the other hand, I think deleting existing entries when we reach code with
disposition of _preserve_existing is surprising, so I'm mostly concerned about
removing surprising behavior, even if we end up doing a little bit more work
because there's a truncated entry.
> 
> [*] Weird script/server code, or even a large request where the user change
its mind and cancels it and then downloads it (yes, that would not result in
StopCaching, but that implies more knowledge about what the downloads code would
or would not do).

True. It's also not a necessary behavior change for this CL. Given everything, I
removed the logic for removing truncated cache entries after StopCaching() is
called.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:2749: entry_disposition = is_sparse_ ||
(mode_ == READ_WRITE && !truncated_)
On 2015/08/19 at 23:46:38, rvargas (slow to respond) wrote:
> is_sparse || mode & read || truncated_ ?.

Code removed.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:2767: entry_disposition = CACHE_ENTRY_DOOM;
On 2015/09/11 at 23:56:17, rvargas (slow to respond) wrote:
> On 2015/09/04 19:09:04, asanka wrote:
> > On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> > > Does this do anything?
> > 
> > DoneWithEntry would only destroy the entry and fire off ERR_CACHE_RACE
errors to
> > pending transactions if this was set. Moved to HttpCache::DoneWithEntry()
> > instead.
> 
> if entry_->doomed is on, the entry will be discarded automatically by the
cache when the last reference is gone.

Yes. But if there are pending transactions, then HttpCache adds the next pending
transaction and proceeds. My sense is that we should invoke the pending
callbacks with ERR_CACHE_RACE and have the next writer re-establish the cache
entry. Is that not the case?

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:2772: if (entry_disposition ==
CACHE_ENTRY_PRESERVE && !is_sparse_ && reading_ &&
On 2015/08/19 at 23:46:38, rvargas (slow to respond) wrote:
> is_sparse != partial_ && !truncated_ (which is what AddTruncatedFlag looks
at).

I moved this condition to AbandonCacheEntry().

https://codereview.chromium.org/1230113012/diff/220001/net/http/disk_based_ce...
File net/http/disk_based_cert_cache_unittest.cc (right):

https://codereview.chromium.org/1230113012/diff/220001/net/http/disk_based_ce...
net/http/disk_based_cert_cache_unittest.cc:48: key,          "", base::Time(),
"",      LOAD_NORMAL, "", "",
On 2015/09/15 at 01:07:38, rvargas (slow to respond) wrote:
> This looks really bad. Can we use the same format of the const mocks and use
one line per member?

This is what 'git cl format' gives us :/

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache.h
File net/http/http_cache.h (right):

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache.h#...
net/http/http_cache.h:351: enum class EntryState { SUCCEEDED, DOOMED };
On 2015/09/11 at 23:56:17, rvargas (slow to respond) wrote:
> nit: types go before methods.

Done.

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache.h#...
net/http/http_cache.h:355: // |entry_state| indicates the state of the entry
after releasing.
On 2015/09/11 at 23:56:17, rvargas (slow to respond) wrote:
> nit: what is "after releasing"?

Clarified.

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache.h#...
net/http/http_cache.h:357: // If |entry_state| is DOOMED, then the cache entry
is destroyed and any
On 2015/09/11 at 23:56:17, rvargas (slow to respond) wrote:
> nit: Can we change this to a command like word instead of past-tense status?.
As in, the caller wants to doom the entry, it's not saying that the entry was
doomed (which it can do by itself).
> 
> (succeed - doom) is a little weird because they don't seem to be on the same
axis.

Changed to KEEP/DOOM

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:465: read_buf_len_ = buf_len;
On 2015/09/11 at 23:56:18, rvargas (slow to respond) wrote:
> Why do we need another member? (vs io_buf_len_)

When we are switching to a single network request, the transaction might need to
overwrite io_buf_len_ to write the truncated bit.

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1495: next_state_ = STATE_NETWORK_READ;
On 2015/09/11 at 23:56:18, rvargas (slow to respond) wrote:
> without returning data to the caller?

We do. Once AbadonCacheEntry() completes, the SM will progress through
DoNetworkRead(). Or am I misunderstanding the question?

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1777: //   dealing with the network.
On 2015/09/11 at 23:56:18, rvargas (slow to respond) wrote:
> Do we really need this comment (and the next one)?

Removed. I suppose those two should be obvious. Although the fact that we only
switch when writing to the cache and not when reading might not be too obvious
to a new reader.

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1799: if (request_->method != "GET") {
On 2015/09/11 at 23:56:18, rvargas (slow to respond) wrote:
> Isn't it easier to check for this in StopCaching?

Check moved there. See earlier confusion about whether StopCaching() could be
called before Start().

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1802: // partial network request to fulfil
this non-GET request. Let's just ignore
On 2015/09/11 at 23:56:17, rvargas (slow to respond) wrote:
> I don't think this ever happens. (partial && !get)

Yeah. The condition we check now is:

  (!partial_ || request->method == GET) && (mode_ & WRITE)

.. which checks that:

  * we are writing to the cache.
  * no need to restart the network transaction (!partial_)
    OR
    method is GET and there's a good chance we'll be able to restart the
transaction.

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1820: // resource.
On 2015/09/11 at 23:56:18, rvargas (slow to respond) wrote:
> This is a pre-requisite for byte range requests

Caveat removed.

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1992: // should retain ownership of entry_
until the transaction is destroyed so
On 2015/09/11 at 23:56:17, rvargas (slow to respond) wrote:
> But now AbandonCacheEntry is called from here.

Comment updated.

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:2005:
AbandonCacheEntry(CompletionCallback());
On 2015/09/11 at 23:56:18, rvargas (slow to respond) wrote:
> This is not doing the same checks that the dtor performs.

Different circumstances.
When we get here, the network transaction is done reading and all data from the
network was written to disk. Hence the condition is trying to determine how
confident we are that the end of the network stream legitimately signalled the
end of the resource.

In the destructor, the state of the network stream isn't known. So the
destructor tries to determine whether the transaction received a legitimate
response at all. If so it attempts to abandon the entry instead of marking it as
DOOMed.

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:2680: ReleaseCacheEntry(mode_ == WRITE ?
EntryState::DOOMED
On 2015/09/11 at 23:56:18, rvargas (slow to respond) wrote:
> wasn't the argument irrelevant for readers? In which case this can pass doomed
all the time.

mode_ == READ_WRITE also keeps the entry.

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:2799: return;
On 2015/09/11 at 23:56:18, rvargas (slow to respond) wrote:
> remove

Done.

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:3040: if (is_sparse_ || done_reading_ ||
!(mode_ & WRITE)) {
On 2015/09/11 at 23:56:18, rvargas (slow to respond) wrote:
> is_sparse != partial_ && !truncated_ (which is what AddTruncatedFlag looks
at).

Condition updated with explanation.

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:3048: if (stopped_caching_ && ((mode_ ==
WRITE && !truncated_) ||
On 2015/09/11 at 23:56:18, rvargas (slow to respond) wrote:
> why write vs rw? why write && truncated_ is ok?

Removed this condition block. This "if" is trying to decide whether
StopCaching() was called on a truncated entry. Incidentally (WRITE && !trucated)
is taken to mean that this was a new cache entry which was stopped halfway. In
the absence of the condition we'd mark it as truncated, which is what we are
doing with the latest patchset.

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:3050: ReleaseCacheEntry(EntryState::DOOMED);
On 2015/09/11 at 23:56:18, rvargas (slow to respond) wrote:
> This deserves a comment (what we're after with the preceding condition).

(removed)

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:3059: if (!callback.is_null()) {
On 2015/09/11 at 23:56:17, rvargas (slow to respond) wrote:
> nit: invert the logic

Done.

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_un...
File net/http/http_cache_unittest.cc (left):

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_un...
net/http/http_cache_unittest.cc:2098: 
On 2015/09/15 at 01:07:39, rvargas (slow to respond) wrote:
> nit: keep this line.

Done.

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_un...
net/http/http_cache_unittest.cc:6750: TEST(HttpCache, StopCachingDeletesEntry) {
On 2015/09/15 at 01:07:39, rvargas (slow to respond) wrote:
> I'm curious about what fails with this test now. Or is it that it's redundant
at this point?

It was testing whether StopCaching() deletes non-resumable requests, which was
redundant because StopCaching() was deleting all truncated entries. I reinstated
the test because StopCaching() is no longer aggressive about deleting truncated
entries.

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_un...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_un...
net/http/http_cache_unittest.cc:390: if
(request->extra_headers.HasHeader("X-Indicate-No-Ranges")) {
On 2015/09/15 at 01:07:38, rvargas (slow to respond) wrote:
> nit: maybe X-No-Ranges ?

Done. And documented the header at line 311.

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_un...
net/http/http_cache_unittest.cc:4386: RETURN_DEFAULT_RANGE_HEADER
INDICATE_NO_RANGES_HEADER EXTRA_HEADER;
On 2015/09/15 at 01:07:39, rvargas (slow to respond) wrote:
> nit: do we really need the new #defines? I feel like the actual header is
clear enough about what's going on here and it actually reads better than a
sequence of long uppercase strings separated by a space.

Hmm. I see what you mean. Should I remove the other #defines as well?

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_un...
net/http/http_cache_unittest.cc:7112: kRangeGET_TransactionOK.url, "GET",
base::Time(), EXTRA_HEADER, 0,
On 2015/09/15 at 01:07:39, rvargas (slow to respond) wrote:
> nit: can we use one line per member here?

git cl format.

asanka

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_transaction_test_util.cc File net/http/http_transaction_test_util.cc (left): https://codereview.chromium.org/1230113012/diff/220001/net/http/http_transaction_test_util.cc#oldcode52 net/http/http_transaction_test_util.cc:52: 0, On 2015/09/15 at 01:07:39, rvargas (slow to respond) ...

5 years, 3 months ago (2015-09-17 22:20:29 UTC) #27

rvargas (doing something else)

Just looking at the comments, not the new code. Sorry for the delay. https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_transaction.cc File ...

5 years, 3 months ago (2015-09-19 01:09:35 UTC) #28

Just looking at the comments, not the new code. Sorry for the delay.

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/1230113012/diff/160001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:2767: entry_disposition = CACHE_ENTRY_DOOM;
On 2015/09/17 21:59:40, asanka wrote:
> On 2015/09/11 at 23:56:17, rvargas (slow to respond) wrote:
> > On 2015/09/04 19:09:04, asanka wrote:
> > > On 2015/08/19 at 23:46:38, rvargas (out until 9-8) wrote:
> > > > Does this do anything?
> > > 
> > > DoneWithEntry would only destroy the entry and fire off ERR_CACHE_RACE
> errors to
> > > pending transactions if this was set. Moved to HttpCache::DoneWithEntry()
> > > instead.
> > 
> > if entry_->doomed is on, the entry will be discarded automatically by the
> cache when the last reference is gone.
> 
> Yes. But if there are pending transactions, then HttpCache adds the next
pending
> transaction and proceeds. My sense is that we should invoke the pending
> callbacks with ERR_CACHE_RACE and have the next writer re-establish the cache
> entry. Is that not the case?

I see what you mean. I agree that the cache should figure out what to do... the
transaction should not be looking at ->doomed.

I think the issue is that doomed is not exactly the same as failure. Today,
there can be an ActiveEntry with a writer and 3 pending tx and then a new
request comes in with mode WRITE. That will doom the active entry and create a
new one for the new tx. When the first request completes, if it succeeds, the
first queued request will hopefully read from it, regardless of being another tx
in progress that is downloading a new version of the resource. I mean, no
failures here mean no transactions going from one ActiveEntry to another. The
logic, btw, is that the 5th request started after the first four, so it should
not affect if requests 2-4 read after request 1 or not, even if the data will be
discarded when request 4 completes.

In other words, if this code wants to do something because there was a previous
failure then there should be a flag on the tx, or something like that.

https://codereview.chromium.org/1230113012/diff/220001/net/http/disk_based_ce...
File net/http/disk_based_cert_cache_unittest.cc (right):

https://codereview.chromium.org/1230113012/diff/220001/net/http/disk_based_ce...
net/http/disk_based_cert_cache_unittest.cc:48: key,          "", base::Time(),
"",      LOAD_NORMAL, "", "",
On 2015/09/17 21:59:40, asanka wrote:
> On 2015/09/15 at 01:07:38, rvargas (slow to respond) wrote:
> > This looks really bad. Can we use the same format of the const mocks and use
> one line per member?
> 
> This is what 'git cl format' gives us :/

Really? If you manually set one per line, cl format will revert to _this_? If
that's the case do you mind filing a bug? (I don't think we have people fixing
these on the Chromium side (certainly there's no component or instructions), but
last time I said so, Sleevi said there was a team in charge)

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1495: next_state_ = STATE_NETWORK_READ;
On 2015/09/17 21:59:40, asanka wrote:
> On 2015/09/11 at 23:56:18, rvargas (slow to respond) wrote:
> > without returning data to the caller?
> 
> We do. Once AbadonCacheEntry() completes, the SM will progress through
> DoNetworkRead(). Or am I misunderstanding the question?

The confusing thing is that we may be doing Start() and we should return to the
caller before jumping to Read. I guess stoped_caching_ guarantees that we are
not in Start anymore and instead we are in Read and I'm used to have reading_
expressing that. But if we are beyond Start, we are inside a Read, why are we
taking the decision after sending a new request? Shouldn't it be either at Read
start or after receiving bytes?

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_un...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_un...
net/http/http_cache_unittest.cc:4386: RETURN_DEFAULT_RANGE_HEADER
INDICATE_NO_RANGES_HEADER EXTRA_HEADER;
On 2015/09/17 21:59:41, asanka wrote:
> On 2015/09/15 at 01:07:39, rvargas (slow to respond) wrote:
> > nit: do we really need the new #defines? I feel like the actual header is
> clear enough about what's going on here and it actually reads better than a
> sequence of long uppercase strings separated by a space.
> 
> Hmm. I see what you mean. Should I remove the other #defines as well?

Do you mean EXTRA_HEADER? I think that was added to try to be explicit about the
subtle difference between the two variants. Which seems like a good thing.

Most requests use "foo" EXTRA_HEADER so at this point it's kind of an expected
pattern in this file. In other words, I don't mind that one (whatever is really
relevant for the text is still explicit and not a macro. I'm looking at line
4414 right now).

https://codereview.chromium.org/1230113012/diff/260001/net/http/http_transact...
File net/http/http_transaction_test_util.cc (right):

https://codereview.chromium.org/1230113012/diff/260001/net/http/http_transact...
net/http/http_transaction_test_util.cc:75:
"http://www.example.com/~foo/bar.html", "GET", base::Time(), "",
ahhrrrgggg. this is ridiculous. Having a tool mess up our code this way
(touching 5 structs and leaving 3 in one format and two in another) is just
crazy.

Let's all bow to the power of the all knowing tool!

</rant>

rvargas (doing something else)

5 years, 3 months ago (2015-09-21 22:06:00 UTC) #29

Looks good. I think the only big thing from PS14 is to clarify the point(s) that
we use to switch to network.

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:465: read_buf_len_ = buf_len;
On 2015/09/17 21:59:41, asanka wrote:
> On 2015/09/11 at 23:56:18, rvargas (slow to respond) wrote:
> > Why do we need another member? (vs io_buf_len_)
> 
> When we are switching to a single network request, the transaction might need
to
> overwrite io_buf_len_ to write the truncated bit.

Acknowledged.

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1355: return 0;
On 2015/09/11 23:56:18, rvargas (slow to respond) wrote:
> nit: OK (or result).

Missed?

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1799: if (request_->method != "GET") {
On 2015/09/17 21:59:41, asanka wrote:
> On 2015/09/11 at 23:56:18, rvargas (slow to respond) wrote:
> > Isn't it easier to check for this in StopCaching?
> 
> Check moved there. See earlier confusion about whether StopCaching() could be
> called before Start().

but PS14 still has the if here...

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:2005:
AbandonCacheEntry(CompletionCallback());
On 2015/09/17 21:59:40, asanka wrote:
> On 2015/09/11 at 23:56:18, rvargas (slow to respond) wrote:
> > This is not doing the same checks that the dtor performs.
> 
> Different circumstances.
> When we get here, the network transaction is done reading and all data from
the
> network was written to disk. Hence the condition is trying to determine how
> confident we are that the end of the network stream legitimately signalled the
> end of the resource.
> 
> In the destructor, the state of the network stream isn't known. So the
> destructor tries to determine whether the transaction received a legitimate
> response at all. If so it attempts to abandon the entry instead of marking it
as
> DOOMed.

Poor wording on my part. The destructor ends up looking at
  reading_ && response_.headers.get() &&
  (response_.headers->response_code() == 200 || partial_)

plus what AddTruncatedFlag looks at:
  partial_, CanResume() and done_reading_

The conditions for AddTruncatedFlag moved to AbandonCacheEntry, and from the
dtor itself reading_ && response_.headers.get() are implied because we are here.
That leaves 200 || partial_ (and partial_ is handled by AddTruncatedFlag).

So the only thing that seems missing is 200. Maybe it doesn't matter? (the 200
is a weak test to only do work for "normal" requests). It looks like we still
need it.

https://codereview.chromium.org/1230113012/diff/220001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:2680: ReleaseCacheEntry(mode_ == WRITE ?
EntryState::DOOMED
On 2015/09/17 21:59:41, asanka wrote:
> On 2015/09/11 at 23:56:18, rvargas (slow to respond) wrote:
> > wasn't the argument irrelevant for readers? In which case this can pass
doomed
> all the time.
> 
> mode_ == READ_WRITE also keeps the entry.

Acknowledged.

https://codereview.chromium.org/1230113012/diff/260001/net/http/http_cache_tr...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/1230113012/diff/260001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:496: stopped_caching_ = true;
It looks like we set the flag when method != GET. I don't think this call makes
sense (atm) for any other method so it should be ignored if that's the case, no?

https://codereview.chromium.org/1230113012/diff/260001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1967: // happen due to a connection problem.
In the latter case the resulting
tiny nit: maybe remove "the resulting" and "call"

https://codereview.chromium.org/1230113012/diff/260001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:3013: // Is the cache entry is sparse or if
the entry is complete? Then the entry
needs some rewording :). I guess you were going for "is foo or is bar?"

Issue 1230113012: [net] Better StopCaching() handling for HttpCache::Transaction. (Closed)

Description

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : Address comments #

Patch Set 5 : Release cache lock early and request open range if it's the final range. #

Patch Set 6 : #

Patch Set 7 : #

Patch Set 8 : #

Patch Set 9 : #

Patch Set 10 : #

Patch Set 11 : #

Patch Set 12 : #

Patch Set 13 : Disallow StopCaching() before Start() and don't delete truncated entries aggressively. #

Patch Set 14 : Add missing MockTransaction initializers #

Messages