Fix race condition for non-open/create operations happening after a doom.

net/disk_cache/simple/simple_entry_operation.cc

 // Copyright 2013 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_entry_operation.h"
 
 #include "base/logging.h"
 #include "net/base/io_buffer.h"
 #include "net/disk_cache/disk_cache.h"
 #include "net/disk_cache/simple/simple_entry_impl.h"
@@ skipping 10 matching lines @@
       type_(other.type_),
       have_index_(other.have_index_),
       index_(other.index_),
       truncate_(other.truncate_),
       optimistic_(other.optimistic_),
       alone_in_queue_(other.alone_in_queue_) {
 }
 
 SimpleEntryOperation::~SimpleEntryOperation() {}
 
-// Static.
+// static
 SimpleEntryOperation SimpleEntryOperation::OpenOperation(
     SimpleEntryImpl* entry,
     bool have_index,
     const CompletionCallback& callback,
     Entry** out_entry) {
   return SimpleEntryOperation(entry,
                               NULL,
                               callback,
                               out_entry,
                               0,
                               0,
                               TYPE_OPEN,
                               have_index,
                               0,
                               false,
                               false,
                               false);
 }
 
-// Static.
+// static
 SimpleEntryOperation SimpleEntryOperation::CreateOperation(
     SimpleEntryImpl* entry,
     bool have_index,
     const CompletionCallback& callback,
     Entry** out_entry) {
   return SimpleEntryOperation(entry,
                               NULL,
                               callback,
                               out_entry,
                               0,
                               0,
                               TYPE_CREATE,
                               have_index,
                               0,
                               false,
                               false,
                               false);
 }
 
-// Static.
+// static
 SimpleEntryOperation SimpleEntryOperation::CloseOperation(
     SimpleEntryImpl* entry) {
   return SimpleEntryOperation(entry,
                               NULL,
                               CompletionCallback(),
                               NULL,
                               0,
                               0,
                               TYPE_CLOSE,
                               false,
                               0,
                               false,
                               false,
                               false);
 }
 
-// Static.
+// static
 SimpleEntryOperation SimpleEntryOperation::ReadOperation(
     SimpleEntryImpl* entry,
     int index,
     int offset,
     int length,
     net::IOBuffer* buf,
     const CompletionCallback& callback,
     bool alone_in_queue) {
   return SimpleEntryOperation(entry,
                               buf,
                               callback,
                               NULL,
                               offset,
                               length,
                               TYPE_READ,
                               false,
                               index,
                               false,
                               false,
                               alone_in_queue);
 }
 
-// Static.
+// static
 SimpleEntryOperation SimpleEntryOperation::WriteOperation(
     SimpleEntryImpl* entry,
     int index,
     int offset,
     int length,
     net::IOBuffer* buf,
     bool truncate,
     bool optimistic,
     const CompletionCallback& callback) {
   return SimpleEntryOperation(entry,
                               buf,
                               callback,
                               NULL,
                               offset,
                               length,
                               TYPE_WRITE,
                               false,
                               index,
                               truncate,
                               optimistic,
                               false);
 }
 

[gavinp, 2013/08/26 20:36:50]

// static

And you were so good everywhere else!

[Philippe, 2013/08/27 11:33:30]

Ahaha :)

+SimpleEntryOperation SimpleEntryOperation::DoomOperation(
+    SimpleEntryImpl* entry,
+    const CompletionCallback& callback) {
+  return SimpleEntryOperation(entry,
+                              NULL,
+                              callback,
+                              NULL,
+                              0,
+                              0,
+                              TYPE_DOOM,
+                              false,

[gavinp, 2013/08/26 20:36:50]

All these booleans make me dizzy. Can you make a const bool with the right name
for each of them, and use it in the call? I prefer that slightly to /*
nameofbool */. I wonder what the right thing to do about all these bools is
though.

[Philippe, 2013/08/27 11:33:30]

Yeah I was also pretty sad to have to add this code especially for some state
that we don't use (and I don't like to pay for what I don't use :)). And I'm
even more sad now with this const variables boilerplate code.

This made me think that an alternative design for SimpleEntryOperation (maybe
less efficient though due to the extra vptr and heap allocation) would be to
have a SimpleEntryOperation abstract base class with the state (e.g. |entry_|,
|callback_|) needed by all the subclasses (the subclasses being CreateOperation,
OpenOperation...). The fact that we would have to heap-allocate the object would
not be a big regression though since we are already doing that to keep track of
the current operation in |executing_operation_|. Then we would have a queue of
pointers rather than a queue of values. There would be an extra indirection
(completely negligible in my opinion) to access the object but there would be
less copy on the other hand when the underlying vector is resized.
Then there is also the question of how we operate on the concrete type when we
are provided with an abstract entry operation. I see two options, my favorite
being the second one:
- keep the type enumeration and provide a cast template method.
- add a visitor interface that simple entry or possibly a private nested
delegate class would implement. This would also let us get rid of the operation
dispatching code that we have in simple_entry_impl.cc.

I would be glad to experiment with this interesting refactoring once we have
less high priority tasks (if this ever happens :))

+                              0,
+                              false,
+                              false,
+                              false);
+}
+
 bool SimpleEntryOperation::ConflictsWith(
     const SimpleEntryOperation& other_op) const {
   if (type_ != TYPE_READ && type_ != TYPE_WRITE)
     return true;
   if (other_op.type() != TYPE_READ && other_op.type() != TYPE_WRITE)
     return true;
   if (type() == TYPE_READ && other_op.type() == TYPE_READ)
     return false;
   if (index_ != other_op.index_)
     return false;
@@ skipping 30 matching lines @@
       length_(length),
       type_(type),
       have_index_(have_index),
       index_(index),
       truncate_(truncate),
       optimistic_(optimistic),
       alone_in_queue_(alone_in_queue) {
 }
 
 }  // namespace disk_cache