Disk cache: Remove all non essential synchronization from the cache destructor.

Disk cache: Remove all non essential synchronization from the
cache destructor.

The default setting for unit tests is still to perform full
synchronization so we make sure that all work is performed, and
that there are no leaks.

However, when not running unit tests, all in progress operations are
simply dropped on the flor, so they should result in dirty entries
for the next run.

BUG=74623
TEST=net_unittests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=127826

[gavinp, 2012-03-16 17:31:35]

I really like removing the namespace qualification on common objects, but I'd
like that split into a separate CL; I'm happy to review both CLs, but having
them together makes this too hard for me to follow.

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

http://codereview.chromium.org/9702059/diff/4011/net/disk_cache/disk_cache.h#...
net/disk_cache/disk_cache.h:61: typedef net::CompletionCallback
CompletionCallback;
This is a great idea! Unfortunately, all of the namespace qualification changes
are making this CL too hard for me to read.

[rvargas (doing something else), 2012-03-16 23:19:38]

I was just fixing the style of the files I was touching (regressed with the bind
stuff).

I removed the style changes.

[gavinp, 2012-03-18 00:35:26]

I didn't get all the way through this.  This CL has serious crashing races that
need to be addressed before it can proceed.

After we fix the ones we've found, I wonder if it's best to hold off on this
under after the m19 branch, for maximum baking time to discover crashes?

http://codereview.chromium.org/9702059/diff/11001/net/disk_cache/backend_impl.cc
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/9702059/diff/11001/net/disk_cache/backend_impl...
net/disk_cache/backend_impl.cc:355: if (user_flags_ & kNoRandom) {
Do these long comments call the name kNoRandom into question?  Perhaps we should
add a flag or rename this one.

http://codereview.chromium.org/9702059/diff/11001/net/disk_cache/backend_impl...
net/disk_cache/backend_impl.cc:362: background_queue_.DropPendingIO();
ignore_result(trace_object_.release());  // Leak the trace_object as it's used
by pending IO.

The problem is this:

1. An IO operation completes, calling Trace(), getting the IP past the check for
    if (!s_trace_buffer || !s_trace_enabled) , meanwhile...

2. BackendImpl::~BackendImpl calls
trace_object_.scoped_refptr<TraceObject>::~scoped_refptr<TraceObject>(),
particular this bit:
    s_trace_buffer = NULL;

3. Our old friend from (1) continues executing Trace(), and evaluates
s_trace_buffer->buffer[blah].  **BOOM**

http://codereview.chromium.org/9702059/diff/11001/net/disk_cache/backend_impl...
net/disk_cache/backend_impl.cc:362: background_queue_.DropPendingIO();
ignore_result(index_.release());  // Do we need this too?

I haven't found a crash on this one.  I suspect one of the fast exit
create-entry tests could, but it hasn't happened to me yet.

http://codereview.chromium.org/9702059/diff/11001/net/disk_cache/backend_impl...
net/disk_cache/backend_impl.cc:362: background_queue_.DropPendingIO();
What what about block_files_ ?  I hacked up a patch to make it a releasable
scoped_ptr (a nice fast-exit idiom), and crashes seemed down.  However, even
with that, I'm still seeing crashes in the main thread in OnOperationComplete()

http://codereview.chromium.org/9702059/diff/11001/net/disk_cache/backend_unit...
File net/disk_cache/backend_unittest.cc (right):

http://codereview.chromium.org/9702059/diff/11001/net/disk_cache/backend_unit...
net/disk_cache/backend_unittest.cc:281: void
DiskCacheBackendTest::BackendShutdownWithPendingIO(bool wait) {
A single bool argument at a call site can be confusing to read.  How about we
declare an enum?

enum PendingIOOptions {
  WAIT_FOR_PENDING_IO,
  DROP_PENDING_IO,
};

http://codereview.chromium.org/9702059/diff/11001/net/disk_cache/backend_unit...
net/disk_cache/backend_unittest.cc:281: void
DiskCacheBackendTest::BackendShutdownWithPendingIO(bool wait) {
How about calling this BackendShutdownWithPendingCreate() ?

http://codereview.chromium.org/9702059/diff/11001/net/disk_cache/backend_unit...
net/disk_cache/backend_unittest.cc:291: uint32 flags = wait ?
disk_cache::kNoRandom : disk_cache::kNone;
uint32 flags = disk_cache::kNoBuffering;
if (pending_io_option == WAIT_FOR_PENDING_IO)
  flags |= disk_cache::kNoRandom;

http://codereview.chromium.org/9702059/diff/11001/net/disk_cache/backend_unit...
net/disk_cache/backend_unittest.cc:327: if (wait) {
Take it or leave it nit: I favour no braces in this case, the vertical
compaction make the relationship easier to read to my eyes

http://codereview.chromium.org/9702059/diff/11001/net/disk_cache/backend_unit...
net/disk_cache/backend_unittest.cc:352: void
DiskCacheBackendTest::BackendShutdownWithPendingIO2(bool wait) {
How about BackendShutdownWithPendingWrites() ???

http://codereview.chromium.org/9702059/diff/11001/net/disk_cache/backend_unit...
net/disk_cache/backend_unittest.cc:403: void
DiskCacheBackendTest::BackendShutdownWithPendingIO3(bool wait) {
BackendShutdownWithPendingBufferedCreates ?

http://codereview.chromium.org/9702059/diff/11001/net/disk_cache/in_flight_io.cc
File net/disk_cache/in_flight_io.cc (right):

http://codereview.chromium.org/9702059/diff/11001/net/disk_cache/in_flight_io...
net/disk_cache/in_flight_io.cc:93: DCHECK(io_list_.find(operation) !=
io_list_.end());
  scoped_refptr<BackgroundIO> protect(operation);

http://codereview.chromium.org/9702059/diff/11001/net/disk_cache/in_flight_io...
net/disk_cache/in_flight_io.cc:94:
io_list_.erase(make_scoped_refptr(operation));
You want protect from above, since on a posix system, you will be sad when you
start running the unit test ShutdownWithPendingIO2_Fast.  Here's what might
happen:

1. On the main thread, In DiskCacheBackendTest::BackendShutdownWithPendingIO2(),
call BackendIO::WriteEntry(), which posts a task to the background thread.  We
keep running, and call BackendImpl::~BackendImpl() before anything posts to the
background_thread_.

2. On background_thread_, we start running one of the WriteDataImpl task posted
in (1).

3. On the main thread, continue BackendShutdownWithPendingIO2(), try to delete
our cache_thread automatic, but we can't, so we thread join it while waiting for
the task in (2) to finish.

4. On the background_thread_, the task from (2) calls the async interface to
File::Write(), which calls FileInFlightIO::PostWrite(), which posts a task on a
worker thread to call FileBackgroundIO::Write() and then adds the
FileBackgroundIO operation to the list in the FileInFlightIO object.

5.. On a worker thread, The task FileBackgroundIO::Write() runs, which calls
NotifyController() and eventually posts a task to the callback thread to signal
the background_thread_ to run OnIOSignalled().

6. On the background_thread_, we finish our CreateEntryImpl() task, and delete
the background_thread_, letting the main thread from leave the thread join it
was in.  When we delete the thread, we of course kill the scheduled
OnIOSignalled() task, which leaves only one ref to our FileBackgroundIO object,
the ref held by the FileInFlightIO::io_list_.

7. The main thread calls DiskCacheTestWithCache::TearDown() which calls
CheckCacheIntegrity(), which eventually calls InFlightIO::WaitForPendingIO(),
which calls InFlightIO::InvokeCallback().  When control reaches the line above
to remove the operation from the io_list_, that removes the last ref to the
operation, and deletes it...

... wait for it ....

http://codereview.chromium.org/9702059/diff/11001/net/disk_cache/in_flight_io...
net/disk_cache/in_flight_io.cc:95: OnOperationComplete(operation, cancel_task);
*BOOM*

[rvargas (doing something else), 2012-03-20 02:54:23]

Crashes _should_ be gone now.

http://codereview.chromium.org/9702059/diff/11001/net/disk_cache/backend_impl.cc
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/9702059/diff/11001/net/disk_cache/backend_impl...
net/disk_cache/backend_impl.cc:355: if (user_flags_ & kNoRandom) {
On 2012/03/18 00:35:26, gavinp wrote:
> Do these long comments call the name kNoRandom into question?  Perhaps we
should
> add a flag or rename this one.

I hear you, but I guess I'd have a comment here regardless of the name of the
flag.

Seriously though, using kNoRandom is weird (and we've been using it this way for
other things), but having a flag specifically for unit tests would not be
correct either: that would be set for the _Fast tests anyway. The other option
would be have separate flags for everything (as in a flag for fast shutdown),
but that's an explosion of flags (some set by default) so it may not be the best
thing either.

[gavinp, 2012-03-20 15:40:08]

LGTM (with the one minor nit fixed); I like the way you fixed the races without
leaking.  I will followup with some more DCHECKs in a short upcoming patch.

Coordinate with the memory sheriffs when you land this; the fast exit path leaks
memory in ShutdownWithPendingIO and in ShutdownWithPendingFileIO.

http://codereview.chromium.org/9702059/diff/23001/net/disk_cache/backend_impl.cc
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/9702059/diff/23001/net/disk_cache/backend_impl...
net/disk_cache/backend_impl.cc:512: File::DropPendingIO();
This belt and suspenders approach looks good.

http://codereview.chromium.org/9702059/diff/23001/net/disk_cache/file.h
File net/disk_cache/file.h (right):

http://codereview.chromium.org/9702059/diff/23001/net/disk_cache/file.h#newco...
net/disk_cache/file.h:71: // Drops current pending operation without waiting for
them to complete.
Nit: // Drops current pending operations without waiting for them to complete.

[rvargas (doing something else), 2012-03-20 18:52:17]

Thanks for the review.

I'm still running try runs and deciding when to check this in. :)