[fsp] Buffer consecutive Write() calls.

chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.h

+// 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.
+
+#ifndef CHROME_BROWSER_CHROMEOS_FILE_SYSTEM_PROVIDER_FILEAPI_BUFFERING_FILE_STREAM_WRITER_H_
+#define CHROME_BROWSER_CHROMEOS_FILE_SYSTEM_PROVIDER_FILEAPI_BUFFERING_FILE_STREAM_WRITER_H_
+
+#include "base/basictypes.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "webkit/browser/fileapi/file_stream_writer.h"
+
+namespace net {
+class IOBuffer;
+}  // namespace net
+
+namespace chromeos {
+namespace file_system_provider {
+
+// Wraps the file stream writer implementation with an intermediate buffer.
+// Writes data from the internal file stream writer in chunks of size at least
+// |intermediate_buffer_length| bytes (or less for the last chunk, or when
+// Flush()
+// is explicitely called).
+//
+// The underlying inner file stream writer *must not* return any values

[hashimoto, 2014/09/11 07:52:16]

nit: Sometimes the writer is referred as "inner" writer, but sometimes as
"internal".
Please stick to one word.

[mtomasz, 2014/09/17 04:59:40]

Done.

+// synchronously. Instead, results must be returned by a callback, including
+// errors. Moreover, partial writes are *not* supported.
+//
+// Flush() must be called on completion to ensure that all bytes are written

[hashimoto, 2014/09/11 07:52:16]

This description should be put in storage::FileStreamWriter's header.

This class is used mainly with storage::FileStreamWriter*, not with
BufferingFileStreamWriter*.
So the must-be-flushed contract should be forced to all users of
FileStreamWriter.

[mtomasz, 2014/09/17 04:59:40]

I tried a CHECK() and I think this is not the best idea. We would need always to
call Flush() after writing some data to a writer, including on shutdown.

Currently we *do not* call Flush() on shutdown in CopyOrMoveOperationDelegate.
As a result, we'd have a CHECK crash. To avoid, we could add Flush() calls in
the destructor, however this is iffy, since Flush() may be asynchronous.
Especially for this fsp::BufferingFileStreamWriter implementation, flushing is
asynchronous, so it basically can't complete, and that data will be lost.

Yet another issue is tests. If we fail on ASSERT, then the test case will abort
without calling Flush(). We'd crash. To avoid it we would have to create a
wrapper over BufferingFileStreamWriter, which would call Flush() from the
destructor. It is overcomplicated.

Instead, I went in the end with with the second solution we discussed before. I
call fsp::BufferingFileStreamWriter::FlushIntermediateBuffer() from the
destructor of fsp::BufferingFileStreamWriter. As a result, we don't need to add
a special assumption to storage::FileStreamWriter about flushing. The data will
be always automatically flushed when a fsp::BufferingFileStreamWriter object is
destroyed. As for shutdown, we'd lose data anyway, since it's impossible to
flush at the shutdown time.

[hashimoto, 2014/09/22 12:10:31]

You can replace CHECK with something harmless like LOG(ERROR) with #if
defined(UNIT_TEST).
Calling Flush() in the dtor is not helpful especially when the system is
shutdown just after the Flush() call. (it might result in a tricky race bug)
Can't we do something to distinguish intended data loss (e.g. shutdown during
copy) from a bug where the developer forgets to call Flush()? (e.g. Calling
Flush() in the dtor of CopyOrMoveOperationDelegate())

[mtomasz, 2014/09/24 01:45:31]

That's possible, but I think flushing from the destructor is just simpler. We
don't need to add a requirement to Flush() after copying which you were against.
So there is no way we have a programming error by not calling Flush() you
mentioned later.
What race do you have in mind? I fixed a possible race in the latest patchset.
There is no need to call Flush() anymore because adding the flushing logic to
the destructor. So we will only lose data on shutdown, which as you said is
intended.

[hashimoto, 2014/09/26 07:05:55]

Flush() in the dtor requires the provider process to stay alive for a sufficient
amount of time after it gets called.

I'm concerned about cases like the following:
1. A file operation finishes successfully, with Flush() being called in the
dtor.
2. Wait for a moment.
3. User turns off the device.
If Flush() completes during the step #2, there is no data loss.
But if Flush() cannot finish during #2, the data will be lost.
I guess this should result in a bug which is quite difficult to reproduce.

+// successfully.
+class BufferingFileStreamWriter : public storage::FileStreamWriter {
+ public:
+  BufferingFileStreamWriter(
+      scoped_ptr<storage::FileStreamWriter> file_stream_writer,
+      int intermediate_buffer_length);
+
+  virtual ~BufferingFileStreamWriter();
+
+  // storage::FileStreamWriter overrides.
+  virtual int Write(net::IOBuffer* buf,
+                    int buf_len,
+                    const net::CompletionCallback& callback) OVERRIDE;
+  virtual int Cancel(const net::CompletionCallback& callback) OVERRIDE;
+  virtual int Flush(const net::CompletionCallback& callback) OVERRIDE;
+
+ private:
+  // Copies |length| bytes of data from the |buffer| starting at |offset|
+  // position to the current position of the intermediate buffer.
+  void CopyToIntermediateBuffer(scoped_refptr<net::IOBuffer> buffer,
+                                int offset,
+                                int length);
+
+  // Flushes all of the bytes in the intermediate buffer to the internal
+  // file stream writer.
+  void FlushIntermediateBuffer(const net::CompletionCallback& callback);
+
+  // Called when flushing the intermediate buffer is completed with either
+  // a success or an error.
+  void OnFlushIntermediateBufferCompleted(
+      int length,
+      const net::CompletionCallback& callback,
+      int result);
+
+  // Called when flushing the intermediate buffer for direct write is completed
+  // with either a success or an error.
+  void OnFlushIntermediateBufferForDirectWriteCompleted(
+      scoped_refptr<net::IOBuffer> buffer,
+      int length,
+      const net::CompletionCallback& callback,
+      int result);
+
+  // Called when flushing the intermediate buffer for a buffered write is
+  // completed with either a success or an error.
+  void OnFlushIntermediateBufferForBufferedWriteCompleted(
+      scoped_refptr<net::IOBuffer> buffer,
+      int buffered_bytes,
+      int bytes_left,
+      const net::CompletionCallback& callback,
+      int result);
+
+  // Called when flushing the intermediate buffer for a flush call is completed
+  // with either a success or an error.
+  void OnFlushIntermediateBufferForFlushCompleted(
+      const net::CompletionCallback& callback,
+      int result);
+
+  scoped_ptr<storage::FileStreamWriter> file_stream_writer_;
+  int intermediate_buffer_length_;
+  scoped_refptr<net::IOBuffer> intermediate_buffer_;
+  int buffered_bytes_;
+
+  base::WeakPtrFactory<BufferingFileStreamWriter> weak_ptr_factory_;
+  DISALLOW_COPY_AND_ASSIGN(BufferingFileStreamWriter);
+};
+
+}  // namespace file_system_provider
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_FILE_SYSTEM_PROVIDER_FILEAPI_BUFFERING_FILE_STREAM_WRITER_H_