Deflake Simple Cache backend on Windows.

net/disk_cache/simple/simple_util_win.cc

Index: net/disk_cache/simple/simple_util_win.cc
diff --git a/net/disk_cache/simple/simple_util_win.cc b/net/disk_cache/simple/simple_util_win.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d0d191dbacdd935defd5ac478cc97d6703f23d5a
--- /dev/null
+++ b/net/disk_cache/simple/simple_util_win.cc
@@ -0,0 +1,44 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/disk_cache/simple/simple_util.h"
+
+#include <windows.h>
+
+#include "base/files/file_util.h"
+#include "base/format_macros.h"
+#include "base/rand_util.h"
+#include "base/strings/stringprintf.h"
+#include "net/disk_cache/cache_util.h"
+
+namespace disk_cache {
+namespace simple_util {
+
+bool SimpleCacheDeleteFile(const base::FilePath& path) {

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

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.

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

Acknowledged.

+  // Even if a file was opened with FLAG_SHARE_DELETE, it is not possible to
+  // create a new file with the same name until the original file is actually

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

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]

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.
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. 
 
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:32]

Got it.
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).
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.

+  // deleted. To allow new files to be created with the new name right away,
+  // the file is renamed before it is deleted.
+
+  // Why a random name? Because if the name was derived from our original name,
+  // then churn on a particular cache entry could cause flakey behaviour.
+
+  // TODO(gaivnp): Ensure these "todelete_" files are cleaned up on periodic
+  // directory sweeps.
+  const base::FilePath rename_target =
+      path.DirName().AppendASCII(base::StringPrintf("todelete_%016" PRIx64,
+                                                    base::RandUint64()));
+
+  bool rename_succeeded = !!MoveFile(path.value().c_str(),
+                                     rename_target.value().c_str());
+  if (rename_succeeded)
+    return DeleteCacheFile(rename_target);
+
+  // The rename did not succeed. The fallback behaviour is to delete the file in
+  // place, which might cause some flake.
+  return DeleteCacheFile(path);
+}
+
+}  // namespace simple_util
+}  // namespace disk_cache