Make DownloadFileImpl handle multiple byte streams.

content/browser/download/download_file_impl.h

Index: content/browser/download/download_file_impl.h
diff --git a/content/browser/download/download_file_impl.h b/content/browser/download/download_file_impl.h
index 9a275190aa5e6cbfbadc36b95839cdafc32a9084..f9b4277048c4011a2d03c7131544c06687643c33 100644
--- a/content/browser/download/download_file_impl.h
+++ b/content/browser/download/download_file_impl.h
@@ -12,11 +12,13 @@
 
 #include <memory>
 #include <string>
+#include <unordered_map>
 
 #include "base/files/file.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "base/threading/thread_checker.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "content/browser/byte_stream.h"
@@ -41,14 +43,19 @@ class CONTENT_EXPORT DownloadFileImpl : public DownloadFile {
   // DownloadDestinationObserver.
   DownloadFileImpl(std::unique_ptr<DownloadSaveInfo> save_info,
                    const base::FilePath& default_downloads_directory,
-                   std::unique_ptr<ByteStreamReader> byte_stream,
+                   std::unique_ptr<ByteStreamReader> stream_reader,
                    const net::NetLogWithSource& net_log,
+                   bool is_sparse_file,
                    base::WeakPtr<DownloadDestinationObserver> observer);
 
   ~DownloadFileImpl() override;
 
   // DownloadFile functions.
   void Initialize(const InitializeCallback& callback) override;
+
+  void AddByteStream(std::unique_ptr<ByteStreamReader> stream_reader,
+                     int64_t offset) override;
+
   void RenameAndUniquify(const base::FilePath& full_path,
                          const RenameCompletionCallback& callback) override;
   void RenameAndAnnotate(const base::FilePath& full_path,
@@ -63,9 +70,17 @@ class CONTENT_EXPORT DownloadFileImpl : public DownloadFile {
 
  protected:
   // For test class overrides.
+  // Append data to the file.
+  // On OS level, it will write at current position to the file.
   virtual DownloadInterruptReason AppendDataToFile(
       const char* data, size_t data_len);
 
+  // Write data from the offset to the file.
+  // On OS level, it will seek to the |offset| and write from there.
+  DownloadInterruptReason WriteDataToFile(int64_t offset,
+                                          const char* data,
+                                          size_t data_len);
+
   virtual base::TimeDelta GetRetryDelayForFailedRename(int attempt_number);
 
   virtual bool ShouldRetryFailedRename(DownloadInterruptReason reason);
@@ -73,6 +88,56 @@ class CONTENT_EXPORT DownloadFileImpl : public DownloadFile {
  private:
   friend class DownloadFileTest;
 
+  // Wrapper of a ByteStreamReader, and the meta data needed to write to a
+  // slice of the target file.
+  //
+  // Does not require the stream reader ready when constructor is called.
+  // |stream_reader_| can be set later when the network response is handled.
+  //
+  // Multiple SourceStreams can concurrently write to the same file sink.
+  //
+  // The file IO processing is finished when all SourceStreams are finished.
+  class CONTENT_EXPORT SourceStream {
+   public:
+    SourceStream(int64_t offset, int64_t bytes_written);

[qinmin, 2017/02/28 06:56:12]

why does this take bytes_written? Shouldn't that always be 0 when the stream is
just constructed?

On the other hand, the SourceStream ctor will need a length param to know how
much data it can write to the file. Otherwise, the total bytes written will be
incorrect.

[xingliu, 2017/02/28 23:24:21]

bytes_written can be serialized/deserialized from/to slice info, and used to
figure out total bytes. But it's a bit weird that fully downloaded slice should
also create a finished SourceStream in this way.

If we don't cache bytes written in SourceStream, then we probably need to
maintain a list of slice info in DownloadFileImpl. I'm fine with that too, so
SourceStream is only for non-completed slice.
wdyt?


Stream doesn't need to know the length, the request should have the correct
range already, assuming the server gives the correct data. The idea is mostly
following the current implementation that DownloadFileImpl doesn't check
content-length to complete the download. When the stream is finished it's done,
which is controlled by the server.

[qinmin, 2017/03/01 00:01:12]

If we start downloding a new slice, it should come with a new offset and 0
bytes_written.  If Chrome crashes when downloading a slice, it can always create
a new slice for the remainder of the data when it restarts. And the new slice
will also come with a new offset and 0 bytes written.

The slice info should be passed into download file, so we can check the file
integrity during write.

There is no guarantee that a server will correctly handle a range request. If
that happens, this class will continue to write even if it already write
|length| bytes.

[xingliu, 2017/03/01 19:25:07]

Done, bytes written now always starts from 0 for a new request.
Added length to limit the bytes written. Added a unit test for limit length.

+    ~SourceStream();
+
+    void SetByteStream(std::unique_ptr<ByteStreamReader> stream_reader);
+
+    // Called when successfully read a buffer from the stream and write it to
+    // disk.
+    void OnWriteBytesToDisk(int64_t bytes_write);
+
+    ByteStreamReader* stream_reader() const { return stream_reader_.get(); }
+    int64_t offset() const { return offset_; }
+    int64_t bytes_written() const { return bytes_written_; }
+    bool is_finished() const { return finished_; }
+    void set_finished(bool finish) { finished_ = finish; }
+
+   private:
+    // Starting position for the current slice.
+    int64_t offset_;
+
+    // Number of bytes written to disk for the slice. The initial value may be
+    // read from download history.
+    // Will write to disk at (|offset_| + |bytes_received_|).
+    int64_t bytes_written_;
+
+    // If all the data read from the stream has been successfully write to disk.
+    bool finished_;
+
+    // The stream through which data comes.
+    // TODO(rdsmith): Move this into BaseFile; requires using the same
+    // stream semantics in SavePackage.  Alternatively, replace SaveFile
+    // with DownloadFile and get rid of BaseFile.
+    std::unique_ptr<ByteStreamReader> stream_reader_;
+
+    DISALLOW_COPY_AND_ASSIGN(SourceStream);
+  };
+
+  typedef std::unordered_map<int64_t, std::unique_ptr<SourceStream>>
+      SourceStreams;
+
   // Options for RenameWithRetryInternal.
   enum RenameOption {
     UNIQUIFY = 1 << 0,  // If there's already a file on disk that conflicts with
@@ -108,9 +173,20 @@ class CONTENT_EXPORT DownloadFileImpl : public DownloadFile {
   // Send an update on our progress.
   void SendUpdate();
 
-  // Called when there's some activity on stream_reader_ that needs to be
+  // Called before the data is written to disk.
+  void WillWriteToDisk(size_t data_len);
+
+  // Called when there's some activity on the byte stream that needs to be
   // handled.
-  void StreamActive();
+  void StreamActive(SourceStream* source_stream);
+
+  // Register callback and start to read data from the stream.
+  void RegisterAndActivateStream(SourceStream* source_stream);
+
+  // Return the total valid bytes received in the target file.
+  // If the file is a sparse file, return the total number of valid bytes.
+  // Otherwise, return the current file size.
+  int64_t TotalBytesReceived() const;
 
   net::NetLogWithSource net_log_;
 
@@ -125,23 +201,29 @@ class CONTENT_EXPORT DownloadFileImpl : public DownloadFile {
   // The default directory for creating the download file.
   base::FilePath default_download_directory_;
 
-  // The stream through which data comes.
-  // TODO(rdsmith): Move this into BaseFile; requires using the same
-  // stream semantics in SavePackage.  Alternatively, replace SaveFile
-  // with DownloadFile and get rid of BaseFile.
-  std::unique_ptr<ByteStreamReader> stream_reader_;
+  // Map of the offset and the source stream that represents the slice
+  // starting from offset.
+  // Must be modified on the same thread that constructs the DownloadFile.
+  // Any byte stream should have a SourceStream before added to the download
+  // file.
+  // The disk IO is completed when all source streams are finished.
+  SourceStreams source_streams_;
 
   // Used to trigger progress updates.
   std::unique_ptr<base::RepeatingTimer> update_timer_;
 
+  // Set to true when multiple byte streams write to the same file.
+  // The file may contains null bytes(holes) in between of valid data slices.
+  bool is_sparse_file_;
+
   // Statistics
   size_t bytes_seen_;
   base::TimeDelta disk_writes_time_;
   base::TimeTicks download_start_;
   RateEstimator rate_estimator_;
 
+  base::ThreadChecker thread_checker_;
   base::WeakPtr<DownloadDestinationObserver> observer_;
-
   base::WeakPtrFactory<DownloadFileImpl> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadFileImpl);