Disk cache: Add a method to cancel pending sparse operations....

Disk cache: Add a method to cancel pending sparse operations.

The sparse IO methods require exclusive use of the cache entry
and they complain when that requirement is violated. When the
user cancels a request and reissues another one to the same entry,
we may be waiting for the previous operation to finish when we
receive a new IO request, so we fail.

This CL add a way for the HTTP cache to cancel IO operations and
get a notification when the disk cache is able to operate on that
entry again.

BUG=23862
TEST=unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=28475

[rvargas (doing something else), 2009-10-07 18:34:27]

Wan-Teh: Please review the http cache.

Nicolas: Please review the disk cache.

[Nicolas Sylvain, 2009-10-07 20:36:45]

LGTM

http://codereview.chromium.org/256090/diff/4001/5003
File net/disk_cache/sparse_control.cc (right):

http://codereview.chromium.org/256090/diff/4001/5003#newcode733
Line 733: if (i == abort_callbacks_.size() - 1)
maybe a comment explaining that we can't do that after the last
entry_->Release() ? but maybe it should be obvious?

[wtc, 2009-10-08 02:20:03]

LGTM.  Nice comments and unit tests as usual!

http://codereview.chromium.org/256090/diff/4001/5008
File net/disk_cache/disk_cache.h (right):

http://codereview.chromium.org/256090/diff/4001/5008#newcode224
Line 224: // finishes, but that event will hopefully happen sooner when this
method is
I don't understand the "hopefully" sentence.  We should
document the behavior of this function assertively.

http://codereview.chromium.org/256090/diff/4001/5008#newcode230
Line 230: // entry is ready to use. This method always returns true for
non-sparse
"returns true" is wrong.  Do you mean "returns OK"?

http://codereview.chromium.org/256090/diff/4001/5008#newcode234
Line 234: // true OK although another IO operation cannot be issued at this
time; in
Typo: true OK

If this method can only return OK or ERR_IO_PENDING, why
don't we let it return bool?

Nit: should we rename this method MarkReadyToUse?
ReadyToUse sounds like a function that returns a bool.

http://codereview.chromium.org/256090/diff/4001/5006
File net/disk_cache/mem_entry_impl.h (right):

http://codereview.chromium.org/256090/diff/4001/5006#newcode72
Line 72: virtual int ReadyToUse(net::CompletionCallback* completion_callback);
Should we inline this method, too?  (It simply returns OK.)

http://codereview.chromium.org/256090/diff/4001/5003
File net/disk_cache/sparse_control.cc (right):

http://codereview.chromium.org/256090/diff/4001/5003#newcode242
Line 242: // release it before invoking user_callback_.
user_callback_ => completion_callback or abort_callbacks_?

http://codereview.chromium.org/256090/diff/4001/5003#newcode704
Line 704: // We'll return the current result_ of the operation, which may be
less than
Is result_ (with the trailing underscore) a typo?

http://codereview.chromium.org/256090/diff/4001/5003#newcode728
Line 728: if (abort_callbacks_.empty())
This is not necessary because the for loop takes care of
it.

http://codereview.chromium.org/256090/diff/4001/5004
File net/disk_cache/sparse_control.h (right):

http://codereview.chromium.org/256090/diff/4001/5004#newcode72
Line 72: // again. see disk_cache::Entry::ReadyToUse() for more info.
Nit: capitalize "see".

http://codereview.chromium.org/256090/diff/4001/5002
File net/http/http_cache.cc (right):

http://codereview.chromium.org/256090/diff/4001/5002#newcode384
Line 384: // Called to signal completion of the cache's ReadyToUse method:
Nit: cache's => cache entry's ?

[rvargas (doing something else), 2009-10-08 03:03:05]

Thanks for all the comments. I'll address everything with a new snapshot, but
before doing that I have some questions.

http://codereview.chromium.org/256090/diff/4001/5008
File net/disk_cache/disk_cache.h (right):

http://codereview.chromium.org/256090/diff/4001/5008#newcode224
Line 224: // finishes, but that event will hopefully happen sooner when this
method is
On 2009/10/08 02:20:03, wtc wrote:
> I don't understand the "hopefully" sentence.  We should
> document the behavior of this function assertively.

I'll remove that word. It was there because it is possible for the operation top
take the same time with or without this call, but in any case the method should
be used when deleting the http cache transaction.

http://codereview.chromium.org/256090/diff/4001/5008#newcode234
Line 234: // true OK although another IO operation cannot be issued at this
time; in
On 2009/10/08 02:20:03, wtc wrote:
> Typo: true OK
> 
> If this method can only return OK or ERR_IO_PENDING, why
> don't we let it return bool?

I wrote the code with this method returning bool (hence the remaining typos),
but when I showed the interface to Darin he suggested changing it to int so that
it was closer to all other methods that take a callback.

I also considered multiple alternate names (because the association of
ReadyToUse to a bool returning method), but I really couldn't find something
that I liked better. I'd feel that MarkReadyToUse also implies something
different to what we really do.

I could go back to a bool method :( ... as in the first patch set of this cl.

By the way, there is this idea of making all methods async at some point, and in
that case this method will disappear because we can use the OpenEntry()
callback. Of course, the time frame for that change is not clear.

http://codereview.chromium.org/256090/diff/4001/5006
File net/disk_cache/mem_entry_impl.h (right):

http://codereview.chromium.org/256090/diff/4001/5006#newcode72
Line 72: virtual int ReadyToUse(net::CompletionCallback* completion_callback);
On 2009/10/08 02:20:03, wtc wrote:
> Should we inline this method, too?  (It simply returns OK.)

Totally up to you :). There is no performance difference because the method is
virtual, but I'm fine either way.

http://codereview.chromium.org/256090/diff/4001/5003
File net/disk_cache/sparse_control.cc (right):

http://codereview.chromium.org/256090/diff/4001/5003#newcode704
Line 704: // We'll return the current result_ of the operation, which may be
less than
On 2009/10/08 02:20:03, wtc wrote:
> Is result_ (with the trailing underscore) a typo?

It was not, but I'll remove it.

[darin (slow to review), 2009-10-08 06:46:26]

http://codereview.chromium.org/256090/diff/4001/5008
File net/disk_cache/disk_cache.h (right):

http://codereview.chromium.org/256090/diff/4001/5008#newcode226
Line 226: virtual void CancelSparseIO() = 0;
what happened to eliminating this in favor of piggybacking on close?

http://codereview.chromium.org/256090/diff/4001/5008#newcode234
Line 234: // true OK although another IO operation cannot be issued at this
time; in
the set of possible return values should match the set of possible values that
can be conveyed via the completion callback.  the exception is ERR_IO_PENDING,
which cannot be passed to a completion callback.  i think this symmetry is
important to maintain.  otherwise, if we want this method to return bool, then
completion callback is the wrong parameter type.

how about calling this method ReadyForSparseIO to indicate that it is only
relevant if you want to do sparse IO on this entry?

[wtc, 2009-10-08 18:45:19]

http://codereview.chromium.org/256090/diff/4001/5008
File net/disk_cache/disk_cache.h (right):

http://codereview.chromium.org/256090/diff/4001/5008#newcode234
Line 234: // true OK although another IO operation cannot be issued at this
time; in
On 2009/10/08 06:46:26, darin wrote:
> the set of possible return values should match the set of possible values that
> can be conveyed via the completion callback.

Agreed.  I know why this method should return int now.

Ricardo, if you can't come up with a better name, don't
worry about it.  The comments clearly specify what this
method does.

[rvargas (doing something else), 2009-10-08 20:42:53]

http://codereview.chromium.org/256090/diff/4001/5008
File net/disk_cache/disk_cache.h (right):

http://codereview.chromium.org/256090/diff/4001/5008#newcode226
Line 226: virtual void CancelSparseIO() = 0;
On 2009/10/08 06:46:26, darin wrote:
> what happened to eliminating this in favor of piggybacking on close?

After looking at the details it turns out that I routinely use the public API of
an entry (in other words Close) so we would have to change all other users to be
able to tell apart a close that should attempt to cancel from one that doesn't.
That's why I abandoned the idea.

http://codereview.chromium.org/256090/diff/4001/5008#newcode234
Line 234: // true OK although another IO operation cannot be issued at this
time; in
On 2009/10/08 06:46:26, darin wrote:
> how about calling this method ReadyForSparseIO to indicate that it is only
> relevant if you want to do sparse IO on this entry?

I was leaning towards the "from the point of view of the disk cache, it is not
an error to call ReadyToUse for any type of entry; it will simply return OK for
regular entries". I'll change it to this one if there is no other suggestion.

[rvargas (doing something else), 2009-10-08 21:36:48]

New version with all the changes.

http://codereview.chromium.org/256090/diff/4001/5006
File net/disk_cache/mem_entry_impl.h (right):

http://codereview.chromium.org/256090/diff/4001/5006#newcode72
Line 72: virtual int ReadyToUse(net::CompletionCallback* completion_callback);
Actually, I'm leaving it this way because all other members of the interface are
implemented in the cc file (including single liners).

http://codereview.chromium.org/256090/diff/4001/5003
File net/disk_cache/sparse_control.cc (right):

http://codereview.chromium.org/256090/diff/4001/5003#newcode242
Line 242: // release it before invoking user_callback_.
On 2009/10/08 02:20:03, wtc wrote:
> user_callback_ => completion_callback or abort_callbacks_?

Actually, I meant that the reference that we already hold will be released
before calling user_callback_. I reworded this a little.

[wtc, 2009-10-08 21:48:49]

LGTM.

http://codereview.chromium.org/256090/diff/5012/4011
File net/disk_cache/disk_cache.h (right):

http://codereview.chromium.org/256090/diff/5012/4011#newcode224
Line 224: // finishes, but that event will happen sooner when this method is
used.
"that event" is not clear.

Maybe change
  that event will happen sooner
to
  the operation will finish sooner