Deflake Simple Cache backend on Windows.

Deflake Simple Cache backend on Windows.

By taking a bit of care around opening files and deleting them, POSIX-like semantics are possible, and Simple Cache tests pass.

R=gavinp@chromium.org
BUG=None

Committed: https://crrev.com/4b8931c7bea78530879a0e610be2ab0deb3b6384
Cr-Commit-Position: refs/heads/master@{#301784}

[gavinp, 2014-10-28 19:15:07]

Patchset #6 (id:100001) has been deleted

[gavinp, 2014-10-28 19:15:30]

gavinp@chromium.org changed reviewers:
+ jkarlin@chromium.org, mmenke@chromium.org, pasko@chromium.org
- gavinp@chromium.org

[gavinp, 2014-10-28 19:16:52]

mmenke: PTAL

jkarlin, pasko: FYI.

[mmenke, 2014-10-28 20:07:43]

https://codereview.chromium.org/651063002/diff/120001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/651063002/diff/120001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index_file.cc:74: File::FLAG_SHARE_DELETE);
optional:  Think it's much clearer that "File::FLAG_SHARE_DELETE" is a
continuation of the previous line if you intent it 4 spaces.  (Or could move
file_name onto the next line, indented by 4, and then all the flags may fit on
the next line)

https://codereview.chromium.org/651063002/diff/120001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_util.h (right):

https://codereview.chromium.org/651063002/diff/120001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util.h:75: // possible to immediately create a new
file with the same name.
Think it's worth mentioning this requires all open file handles be opened with
File::FLAG_SHARE_DELETE.

https://codereview.chromium.org/651063002/diff/120001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_util_posix.cc (right):

https://codereview.chromium.org/651063002/diff/120001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util_posix.cc:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
nit:  Don't use (c) in new files (x3)

https://codereview.chromium.org/651063002/diff/120001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_util_win.cc (right):

https://codereview.chromium.org/651063002/diff/120001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util_win.cc:18: bool SimpleCacheDeleteFile(const
base::FilePath& path) {
Per offline discussion, this could be a little clearer - add comment, split up
conditional.

[gavinp, 2014-10-28 23:09:40]

Thanks, Matt.

Thank you for your review, I'm very excited about this issue landing.

https://codereview.chromium.org/651063002/diff/120001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/651063002/diff/120001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index_file.cc:74: File::FLAG_SHARE_DELETE);
On 2014/10/28 20:07:43, mmenke wrote:
> optional:  Think it's much clearer that "File::FLAG_SHARE_DELETE" is a
> continuation of the previous line if you intent it 4 spaces.  (Or could move
> file_name onto the next line, indented by 4, and then all the flags may fit on
> the next line)

Done.

https://codereview.chromium.org/651063002/diff/120001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_util.h (right):

https://codereview.chromium.org/651063002/diff/120001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util.h:75: // possible to immediately create a new
file with the same name.
On 2014/10/28 20:07:43, mmenke wrote:
> Think it's worth mentioning this requires all open file handles be opened with
> File::FLAG_SHARE_DELETE.

Done.

https://codereview.chromium.org/651063002/diff/120001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_util_posix.cc (right):

https://codereview.chromium.org/651063002/diff/120001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util_posix.cc:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2014/10/28 20:07:43, mmenke wrote:
> nit:  Don't use (c) in new files (x3)

Done. But x3? Isn't it x2?

[gavinp, 2014-10-28 23:09:59]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-10-28 23:11:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/651063002/140001

[commit-bot: I haz the power, 2014-10-28 23:11:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-28 23:11:11]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[mmenke, 2014-10-28 23:58:37]

LGTM.  Don't care about the others, but should remove the "we's"

https://codereview.chromium.org/651063002/diff/120001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_util_posix.cc (right):

https://codereview.chromium.org/651063002/diff/120001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util_posix.cc:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2014/10/28 23:09:40, gavinp wrote:
> On 2014/10/28 20:07:43, mmenke wrote:
> > nit:  Don't use (c) in new files (x3)
> 
> Done. But x3? Isn't it x2?

Oops...forgot you didn't add simple_util.h in this CL.

https://codereview.chromium.org/651063002/diff/140001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_util_win.cc (right):

https://codereview.chromium.org/651063002/diff/140001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util_win.cc:22: // rename the file to a random name
before deleting it.
nit:  Don't use we in comments.

https://codereview.chromium.org/651063002/diff/140001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util_win.cc:25: // then churn on a particular cache
entry could cause flakey behaviour.
Hrm...I'm not sure if I should whine about valid but less common (In chrome)
spellings here or not (x2).  :)

https://codereview.chromium.org/651063002/diff/140001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util_win.cc:27: // TODO(gaivnp): Ensure we clean up
these "todelete_" files on periodic
nit:  Don't use we.

https://codereview.chromium.org/651063002/diff/140001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util_win.cc:33: const bool rename_succeeded =
MoveFile(path.value().c_str(),
Or if I should whine about the bonus consts, which are even less common in the
Chrome code base.

https://codereview.chromium.org/651063002/diff/140001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util_win.cc:39: // place, which might cause some
flake.
nit:  Don't use we in comments.

[gavinp, 2014-10-29 03:12:26]

Thanks for your review. I've removed the "we".

https://codereview.chromium.org/651063002/diff/120001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_util_posix.cc (right):

https://codereview.chromium.org/651063002/diff/120001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util_posix.cc:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2014/10/28 23:58:36, mmenke wrote:
> On 2014/10/28 23:09:40, gavinp wrote:
> > On 2014/10/28 20:07:43, mmenke wrote:
> > > nit:  Don't use (c) in new files (x3)
> > 
> > Done. But x3? Isn't it x2?
> 
> Oops...forgot you didn't add simple_util.h in this CL.

Acknowledged.

https://codereview.chromium.org/651063002/diff/140001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_util_win.cc (right):

https://codereview.chromium.org/651063002/diff/140001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util_win.cc:22: // rename the file to a random name
before deleting it.
On 2014/10/28 23:58:36, mmenke wrote:
> nit:  Don't use we in comments.

Done.

https://codereview.chromium.org/651063002/diff/140001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util_win.cc:25: // then churn on a particular cache
entry could cause flakey behaviour.
On 2014/10/28 23:58:36, mmenke wrote:
> Hrm...I'm not sure if I should whine about valid but less common (In chrome)
> spellings here or not (x2).  :)

In the library in the Cambridge office, there's a copy of a very good English
dictionary, the "Oxford Canadian English Dictionary." When I look up "behavior"
in it, it says: "variant, chiefly American, of behaviour."

https://codereview.chromium.org/651063002/diff/140001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util_win.cc:27: // TODO(gaivnp): Ensure we clean up
these "todelete_" files on periodic
On 2014/10/28 23:58:36, mmenke wrote:
> nit:  Don't use we.

Done.

https://codereview.chromium.org/651063002/diff/140001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util_win.cc:33: const bool rename_succeeded =
MoveFile(path.value().c_str(),
On 2014/10/28 23:58:36, mmenke wrote:
> Or if I should whine about the bonus consts, which are even less common in the
> Chrome code base.

This is a challenging call. The rule in our coding standard is vague beyond
comprehension: "Use const whenever it makes sense." (well, that really helps).

I have removed this use of const; you're right that it's inconsistent.

https://codereview.chromium.org/651063002/diff/140001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util_win.cc:39: // place, which might cause some
flake.
On 2014/10/28 23:58:36, mmenke wrote:
> nit:  Don't use we in comments.

Done.

[gavinp, 2014-10-29 03:37:23]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-10-29 03:38:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/651063002/180001

[commit-bot: I haz the power, 2014-10-29 04:21:52]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2014-10-29 04:22:40]

Patchset 9 (id:??) landed as
https://crrev.com/4b8931c7bea78530879a0e610be2ab0deb3b6384
Cr-Commit-Position: refs/heads/master@{#301784}

[pasko, 2014-10-29 13:01:02]

flyby LGTM with PHMU (please help me understand)

https://codereview.chromium.org/651063002/diff/180001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/651063002/diff/180001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index_file.cc:362:
simple_util::SimpleCacheDeleteFile(index_filename);
maybe a TODO: it seems it would be cleaner and faster just to overwrite+truncate
the index file with new contents than to move+delete+create+write.

https://codereview.chromium.org/651063002/diff/180001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_util.h (right):

https://codereview.chromium.org/651063002/diff/180001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util.h:74: // this file were opened with
File::FLAG_SHARE_DELETE, it is possible to delete
I would say "Emulates deleting the file", but I'll leave it for your taste to
choose the name.

https://codereview.chromium.org/651063002/diff/180001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_util_win.cc (right):

https://codereview.chromium.org/651063002/diff/180001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util_win.cc:18: bool SimpleCacheDeleteFile(const
base::FilePath& path) {
seems like it better fits to net/disk_cache/cache_util_{win,posix}.cc, there is
really no simplecache specifics here, and I usually prefer not to create new
files. Up to you, both ways are OK with me, combining the files (with renaming
the function to somethins like EmulatePosixCacheFileDeletion) seems slightly
cleaner to me.

https://codereview.chromium.org/651063002/diff/180001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util_win.cc:20: // create a new file with the same
name until the original file is actually
disclaimer: I don't know windows file APIs, can you please educate me a little?

By 'actually deleted' do you mean the windows file api should call us back and
say that the file is deleted? Or maybe it does 'eventually delete' and never
reports back?

Are the move semantics the same regarding the eventual deletion? can it happen
that we move, create, but the file is 'not actually moved yet' when we attempt
to create?

What part of FLAG_SHARE_DELETE semantics are used here? the The file_unittest
checks only that you can FLAG_DELETE_ON_CLOSE with a file successfully when it
was open in the same process with FLAG_SHARE_DELETE, but we are not using
FLAG_DELETE_ON_CLOSE here. Is it OK?

[gavinp, 2014-11-20 15:43:03]

Thanks for the drive by Egor. Since this already landed, I'll consider these
TODOs next time I'm working in the area.

I did notice I spelled my LDAP wrong on a TODO, I'll fix that lest a future grep
miss something.

https://codereview.chromium.org/651063002/diff/180001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/651063002/diff/180001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index_file.cc:362:
simple_util::SimpleCacheDeleteFile(index_filename);
On 2014/10/29 13:01:01, pasko wrote:
> maybe a TODO: it seems it would be cleaner and faster just to
overwrite+truncate
> the index file with new contents than to move+delete+create+write.

Fair point; in this case I suppose we could zero the length; the replacement
should happen later?

https://codereview.chromium.org/651063002/diff/180001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_util_win.cc (right):

https://codereview.chromium.org/651063002/diff/180001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util_win.cc:18: bool SimpleCacheDeleteFile(const
base::FilePath& path) {
On 2014/10/29 13:01:02, pasko wrote:
> seems like it better fits to net/disk_cache/cache_util_{win,posix}.cc, there
is
> really no simplecache specifics here, and I usually prefer not to create new
> files. Up to you, both ways are OK with me, combining the files (with renaming
> the function to somethins like EmulatePosixCacheFileDeletion) seems slightly
> cleaner to me.

Acknowledged.

https://codereview.chromium.org/651063002/diff/180001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util_win.cc:20: // create a new file with the same
name until the original file is actually
On 2014/10/29 13:01:02, pasko wrote:
> disclaimer: I don't know windows file APIs, can you please educate me a
little?
> 
> By 'actually deleted' do you mean the windows file api should call us back and
> say that the file is deleted? Or maybe it does 'eventually delete' and never
> reports back?

If a file is open and it is deleted, then all open handles will be marked as
delete-on-close, and the actual deletion will not happen until the last handle
to the file closes. I don't know if there's a callback; my guess is that there
is one with the filesystem observation API? Not sure.

> 
> Are the move semantics the same regarding the eventual deletion? can it happen
> that we move, create, but the file is 'not actually moved yet' when we attempt
> to create?
>

In theory, yes, since MoveFile is not considered a transacted operation, so we
are being a bit loosey goosey. In practice, a MoveFile inside of the same
directory is atomic on local filesystems, not necessarily on network mounts. 
 
> What part of FLAG_SHARE_DELETE semantics are used here? the The file_unittest
> checks only that you can FLAG_DELETE_ON_CLOSE with a file successfully when it
> was open in the same process with FLAG_SHARE_DELETE, but we are not using
> FLAG_DELETE_ON_CLOSE here. Is it OK?

Yes, it's OK, perhaps I should add a test; deleting an open file through the
filename while there's an open handle is the same as setting
FLAG_DELETE_ON_CLOSE on an already open handle.

[pasko, 2014-11-20 16:19:33]

https://codereview.chromium.org/651063002/diff/180001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/651063002/diff/180001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index_file.cc:362:
simple_util::SimpleCacheDeleteFile(index_filename);
On 2014/11/20 15:43:03, gavinp wrote:
> On 2014/10/29 13:01:01, pasko wrote:
> > maybe a TODO: it seems it would be cleaner and faster just to
> overwrite+truncate
> > the index file with new contents than to move+delete+create+write.
> 
> Fair point; in this case I suppose we could zero the length; the replacement
> should happen later?

yes, truncate+overwrite should also work.

But on the other hand it's probably a moot point. As far as I can tell we flush
the index only twice during the lifetime of the browser process on desktop.

https://codereview.chromium.org/651063002/diff/180001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_util_win.cc (right):

https://codereview.chromium.org/651063002/diff/180001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_util_win.cc:20: // create a new file with the same
name until the original file is actually
On 2014/11/20 15:43:03, gavinp wrote:
> On 2014/10/29 13:01:02, pasko wrote:
> > disclaimer: I don't know windows file APIs, can you please educate me a
> little?
> > 
> > By 'actually deleted' do you mean the windows file api should call us back
and
> > say that the file is deleted? Or maybe it does 'eventually delete' and never
> > reports back?
> 
> If a file is open and it is deleted, then all open handles will be marked as
> delete-on-close, and the actual deletion will not happen until the last handle
> to the file closes.

Got it.

> I don't know if there's a callback; my guess is that there
> is one with the filesystem observation API? Not sure.
> 
> > 
> > Are the move semantics the same regarding the eventual deletion? can it
happen
> > that we move, create, but the file is 'not actually moved yet' when we
attempt
> > to create?
> >
> 
> In theory, yes, since MoveFile is not considered a transacted operation, so we
> are being a bit loosey goosey. In practice, a MoveFile inside of the same
> directory is atomic on local filesystems, not necessarily on network mounts. 

Is this really true? I did not find any proofs of this, not even a marvellous
one.

Some ideas:
http://stackoverflow.com/questions/167414/is-an-atomic-file-rename-with-overw...

namely, when we sunset XP, we can use MoveFileTransacted (may not work on some
Windows Server flavors, I guess).

> > What part of FLAG_SHARE_DELETE semantics are used here? the The
file_unittest
> > checks only that you can FLAG_DELETE_ON_CLOSE with a file successfully when
it
> > was open in the same process with FLAG_SHARE_DELETE, but we are not using
> > FLAG_DELETE_ON_CLOSE here. Is it OK?
> 
> Yes, it's OK, perhaps I should add a test; deleting an open file through the
> filename while there's an open handle is the same as setting
> FLAG_DELETE_ON_CLOSE on an already open handle.

I like the idea to add this test. If we ever are approached with flakiness, we
can see if this test is also flaky, which may (or may not) suggest us the root
cause.