Make SimpleEntryImpl::Doom() completely asynchronous.

Make SimpleEntryImpl::Doom() completely asynchronous.

Over in comments on https://codereview.chromium.org/12192005/ , pasko pointed out that POSIX file semantics allow a fully asynchronous Doom, which is nice, since as Ricardo has pointed out Doom is often called while operations are in flight, and users expect results.

This CL uses that feature to make the Doom operation asynchronous. Note that the existing entry continues to work until closed.

Note: this CL is based on https://codereview.chromium.org/12226095/ and cannot land until after that CL lands. This issue is upstream of https://codereview.chromium.org/12277004/ (make Close() asyncronous) and blocks it from landing.

R=rvargas@chromium.org,felipeg@chromium.org,pasko@chromium.org,willchan@chromium.org
BUG=173392
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184504

[gavinp, 2013-02-11 20:20:05]

Thanks Egor for the suggestion. Ricardo, you are probably happy to see the back
of SimpleSynchronousEntry::DoomAndClose()

[felipeg, 2013-02-12 13:36:55]

lgtm

https://chromiumcodereview.appspot.com/12223075/diff/1/net/disk_cache/simple/...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://chromiumcodereview.appspot.com/12223075/diff/1/net/disk_cache/simple/...
net/disk_cache/simple/simple_entry_impl.cc:177: #if defined(OS_WIN)
For windows we could fallback to the previous implementation (synchronous with
that boolean var).

https://chromiumcodereview.appspot.com/12223075/diff/1/net/disk_cache/simple/...
net/disk_cache/simple/simple_entry_impl.cc:180: // This implementation depends
on POSIX file erasure semantics working.
Maybe adding a little bit more info here would be good.
Such as the comment on your cl.

[pasko-google - do not use, 2013-02-12 14:02:26]

lgtm

[gavinp, 2013-02-12 16:36:30]

Thanks for the reviews!

I saw I had this logic wrong: use the POSIX logic on POSIX systems, not on
nonwindows systems.

https://codereview.chromium.org/12223075/diff/1/net/disk_cache/simple/simple_...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://codereview.chromium.org/12223075/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_entry_impl.cc:177: #if defined(OS_WIN)
On 2013/02/12 13:36:56, felipeg1 wrote:
> For windows we could fallback to the previous implementation (synchronous with
> that boolean var).

Hmmm. But I've removed SimpleSynchronousEntry::DoomAndClose(). Do we really want
too maintain parallel implementations this early?

https://codereview.chromium.org/12223075/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_entry_impl.cc:180: // This implementation depends
on POSIX file erasure semantics working.
On 2013/02/12 13:36:56, felipeg1 wrote:
> Maybe adding a little bit more info here would be good.
> Such as the comment on your cl.

Done.

[felipeg, 2013-02-12 16:38:27]

You are right,
Better not having parallel implementations now.

LGTM


On Tue, Feb 12, 2013 at 5:36 PM, <gavinp@chromium.org> wrote:

> Thanks for the reviews!
>
> I saw I had this logic wrong: use the POSIX logic on POSIX systems, not on
> nonwindows systems.
>
>
> https://codereview.chromium.**org/12223075/diff/1/net/disk_**
>
cache/simple/simple_entry_**impl.cc<https://codereview.chromium.org/12223075/diff/1/net/disk_cache/simple/simple_entry_impl.cc>
> File net/disk_cache/simple/simple_**entry_impl.cc (right):
>
> https://codereview.chromium.**org/12223075/diff/1/net/disk_**
>
cache/simple/simple_entry_**impl.cc#newcode177<https://codereview.chromium.org/12223075/diff/1/net/disk_cache/simple/simple_entry_impl.cc#newcode177>
> net/disk_cache/simple/simple_**entry_impl.cc:177: #if defined(OS_WIN)
>
> On 2013/02/12 13:36:56, felipeg1 wrote:
>
>> For windows we could fallback to the previous implementation
>>
> (synchronous with
>
>> that boolean var).
>>
>
> Hmmm. But I've removed SimpleSynchronousEntry::**DoomAndClose(). Do we
> really want too maintain parallel implementations this early?
>
> https://codereview.chromium.**org/12223075/diff/1/net/disk_**
>
cache/simple/simple_entry_**impl.cc#newcode180<https://codereview.chromium.org/12223075/diff/1/net/disk_cache/simple/simple_entry_impl.cc#newcode180>
>
> net/disk_cache/simple/simple_**entry_impl.cc:180: // This implementation
> depends on POSIX file erasure semantics working.
> On 2013/02/12 13:36:56, felipeg1 wrote:
>
>> Maybe adding a little bit more info here would be good.
>> Such as the comment on your cl.
>>
>
> Done.
>
>
https://codereview.chromium.**org/12223075/<https://codereview.chromium.org/1...
>

[rvargas (doing something else), 2013-02-13 03:23:46]

https://codereview.chromium.org/12223075/diff/11002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://codereview.chromium.org/12223075/diff/11002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:78: // This call to static
SimpleEntryImpl::DoomEntry() will just erase the
I think this is wrong, but it is also wrong on the base CL and should be fixed
over there.

This is sort of a step in the right direction, but ReadData(old_content) must
succeed after doom, not just being able to keep writing (and reading) new
content.

[felipeg, 2013-02-13 13:47:36]

https://codereview.chromium.org/12223075/diff/11002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://codereview.chromium.org/12223075/diff/11002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:78: // This call to static
SimpleEntryImpl::DoomEntry() will just erase the
On 2013/02/13 03:23:46, rvargas wrote:
> I think this is wrong, but it is also wrong on the base CL and should be fixed
> over there.
> 
> This is sort of a step in the right direction, but ReadData(old_content) must
> succeed after doom, not just being able to keep writing (and reading) new
> content.
Hi Ricardo,

Sorry, why ReaData(old_content) will not succeed to read the content that was
previously stored in the file (before being deleted by the Doom call) ?
I think POSIX semantics says that as long as you have the file opened by the
process you can still read the contents even if other process deleted it.
Am I wrong ?

[rvargas (doing something else), 2013-02-13 19:22:53]

On 2013/02/13 13:47:36, felipeg1 wrote:
>
https://codereview.chromium.org/12223075/diff/11002/net/disk_cache/simple/sim...
> File net/disk_cache/simple/simple_entry_impl.cc (right):
> 
>
https://codereview.chromium.org/12223075/diff/11002/net/disk_cache/simple/sim...
> net/disk_cache/simple/simple_entry_impl.cc:78: // This call to static
> SimpleEntryImpl::DoomEntry() will just erase the
> On 2013/02/13 03:23:46, rvargas wrote:
> > I think this is wrong, but it is also wrong on the base CL and should be
fixed
> > over there.
> > 
> > This is sort of a step in the right direction, but ReadData(old_content)
must
> > succeed after doom, not just being able to keep writing (and reading) new
> > content.
> Hi Ricardo,
> 
> Sorry, why ReaData(old_content) will not succeed to read the content that was
> previously stored in the file (before being deleted by the Doom call) ?
> I think POSIX semantics says that as long as you have the file opened by the
> process you can still read the contents even if other process deleted it.
> Am I wrong ?

I was thinking that deletion was a combination of truncate + unlink, but you're
right, just unlinking should work.

[gavinp, 2013-02-14 15:54:09]

Ricardo,

How's this looking?

[rvargas (doing something else), 2013-02-14 20:31:49]

lgtm

[rvargas (doing something else), 2013-02-14 20:32:38]

Same thing about adding the bug to the description.

[gavinp, 2013-02-25 22:52:14]

Committed patchset #10 manually as r184504 (presubmit successful).