Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2712713007:
Make DownloadFileImpl handle multiple byte streams. (Closed)
DescriptionMake DownloadFileImpl handle multiple byte streams.
DownloadFileImpl now hold multiple byte streams, add a wrapper class
for each stream reader.
Didn't hook to slice info or parallel job.
BUG=644352
Review-Url: https://codereview.chromium.org/2712713007
Cr-Commit-Position: refs/heads/master@{#454709}
Committed: https://chromium.googlesource.com/chromium/src/+/992dacf666e6750a1911caebe43b29c93ea4d65f
Patch Set 1 #Patch Set 2 : Export the new class for linking on windows. #
Total comments: 14
Patch Set 3 : Work on feedback. #
Total comments: 4
Patch Set 4 : Work on feedback, add length limitation for stream. #Patch Set 5 : Minor polish to avoid an extra empty read. #
Total comments: 10
Patch Set 6 : Work on feedback. #
Total comments: 28
Patch Set 7 : Work on feedbacks. #Patch Set 8 : Rebase. #
Total comments: 2
Patch Set 9 : Work on feedback. #Patch Set 10 : Remove the AppendDataToFile call, fix the browsertest. #Messages
Total messages: 62 (44 generated)
Description was changed from ========== Make DownloadFileImpl handle multiple byte streams. DownloadFileImpl now hold multiple byte streams, add a wrapper class for each stream reader. ========== to ========== Make DownloadFileImpl handle multiple byte streams. DownloadFileImpl now hold multiple byte streams, add a wrapper class for each stream reader. BUG=644352 ==========
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xingliu@chromium.org changed reviewers: + asanka@chromium.org, dtrainor@chromium.org, qinmin@chromium.org
Hi, please help to take a look, thanks. Tweak the DownloadFileImpl, so it can glue with multiple byte streams from network IO.
Description was changed from ========== Make DownloadFileImpl handle multiple byte streams. DownloadFileImpl now hold multiple byte streams, add a wrapper class for each stream reader. BUG=644352 ========== to ========== Make DownloadFileImpl handle multiple byte streams. DownloadFileImpl now hold multiple byte streams, add a wrapper class for each stream reader. Didn't hook to slice info or parallel job. BUG=644352 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2712713007/diff/20001/content/browser/downloa... File content/browser/download/download_file.h (right): https://codereview.chromium.org/2712713007/diff/20001/content/browser/downloa... content/browser/download/download_file.h:10: #include <memory> remove these newly added includes https://codereview.chromium.org/2712713007/diff/20001/content/browser/downloa... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2712713007/diff/20001/content/browser/downloa... content/browser/download/download_file_impl.cc:115: for (auto& source_stream : source_streams_) { no {} needed https://codereview.chromium.org/2712713007/diff/20001/content/browser/downloa... content/browser/download/download_file_impl.cc:257: if (stream_reader) no {} https://codereview.chromium.org/2712713007/diff/20001/content/browser/downloa... content/browser/download/download_file_impl.cc:328: reason = static_cast<DownloadInterruptReason>( fix the indentations https://codereview.chromium.org/2712713007/diff/20001/content/browser/downloa... content/browser/download/download_file_impl.cc:427: for (auto& stream : source_streams_) { no {} meeded https://codereview.chromium.org/2712713007/diff/20001/content/browser/downloa... File content/browser/download/download_file_impl.h (right): https://codereview.chromium.org/2712713007/diff/20001/content/browser/downloa... content/browser/download/download_file_impl.h:45: class CONTENT_EXPORT SourceStream { does this needs to be public? https://codereview.chromium.org/2712713007/diff/20001/content/browser/downloa... content/browser/download/download_file_impl.h:58: int64_t bytes_received() const { return bytes_received_; } I think byte_received is misleading for a stream, bytes_written is better.
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the review, please take another look. https://codereview.chromium.org/2712713007/diff/20001/content/browser/downloa... File content/browser/download/download_file.h (right): https://codereview.chromium.org/2712713007/diff/20001/content/browser/downloa... content/browser/download/download_file.h:10: #include <memory> On 2017/02/27 18:55:29, qinmin wrote: > remove these newly added includes Done. <memory> is kept for compiling. https://codereview.chromium.org/2712713007/diff/20001/content/browser/downloa... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2712713007/diff/20001/content/browser/downloa... content/browser/download/download_file_impl.cc:115: for (auto& source_stream : source_streams_) { On 2017/02/27 18:55:29, qinmin wrote: > no {} needed Done. https://codereview.chromium.org/2712713007/diff/20001/content/browser/downloa... content/browser/download/download_file_impl.cc:257: if (stream_reader) On 2017/02/27 18:55:29, qinmin wrote: > no {} Done. Kept {} here since there are multiple lines. https://codereview.chromium.org/2712713007/diff/20001/content/browser/downloa... content/browser/download/download_file_impl.cc:328: reason = static_cast<DownloadInterruptReason>( On 2017/02/27 18:55:29, qinmin wrote: > fix the indentations Done. https://codereview.chromium.org/2712713007/diff/20001/content/browser/downloa... content/browser/download/download_file_impl.cc:427: for (auto& stream : source_streams_) { On 2017/02/27 18:55:29, qinmin wrote: > no {} meeded Done. https://codereview.chromium.org/2712713007/diff/20001/content/browser/downloa... File content/browser/download/download_file_impl.h (right): https://codereview.chromium.org/2712713007/diff/20001/content/browser/downloa... content/browser/download/download_file_impl.h:45: class CONTENT_EXPORT SourceStream { On 2017/02/27 18:55:29, qinmin wrote: > does this needs to be public? Makes sense, change it to private. The unit test can access it through friend class. https://codereview.chromium.org/2712713007/diff/20001/content/browser/downloa... content/browser/download/download_file_impl.h:58: int64_t bytes_received() const { return bytes_received_; } On 2017/02/27 18:55:29, qinmin wrote: > I think byte_received is misleading for a stream, bytes_written is better. Done, changed to bytes_written.
https://codereview.chromium.org/2712713007/diff/40001/content/browser/downloa... File content/browser/download/download_file_impl.h (right): https://codereview.chromium.org/2712713007/diff/40001/content/browser/downloa... content/browser/download/download_file_impl.h:102: SourceStream(int64_t offset, int64_t bytes_written); 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.
https://codereview.chromium.org/2712713007/diff/40001/content/browser/downloa... File content/browser/download/download_file_impl.h (right): https://codereview.chromium.org/2712713007/diff/40001/content/browser/downloa... content/browser/download/download_file_impl.h:102: SourceStream(int64_t offset, int64_t bytes_written); On 2017/02/28 06:56:12, qinmin wrote: > 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. 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.
https://codereview.chromium.org/2712713007/diff/40001/content/browser/downloa... File content/browser/download/download_file_impl.h (right): https://codereview.chromium.org/2712713007/diff/40001/content/browser/downloa... content/browser/download/download_file_impl.h:102: SourceStream(int64_t offset, int64_t bytes_written); On 2017/02/28 23:24:21, xingliu wrote: > On 2017/02/28 06:56:12, qinmin wrote: > > 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. > > 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. > 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.
Thanks for the suggestion, PHTAL. https://codereview.chromium.org/2712713007/diff/40001/content/browser/downloa... File content/browser/download/download_file_impl.h (right): https://codereview.chromium.org/2712713007/diff/40001/content/browser/downloa... content/browser/download/download_file_impl.h:102: SourceStream(int64_t offset, int64_t bytes_written); On 2017/03/01 00:01:12, qinmin wrote: > On 2017/02/28 23:24:21, xingliu wrote: > > On 2017/02/28 06:56:12, qinmin wrote: > > > 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. > > > > 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. > > > > 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. 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.
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2712713007/diff/80001/content/browser/downloa... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2712713007/diff/80001/content/browser/downloa... content/browser/download/download_file_impl.cc:367: if (state == ByteStreamReader::STREAM_COMPLETE) { do we need to do this if it is just one stream fails? the original place resets the timer, and it is not done here, https://codereview.chromium.org/2712713007/diff/80001/content/browser/downloa... content/browser/download/download_file_impl.cc:402: weak_factory_.InvalidateWeakPtrs(); ditto: reset timer here? https://codereview.chromium.org/2712713007/diff/80001/content/browser/downloa... content/browser/download/download_file_impl.cc:423: source_stream->stream_reader()->RegisterCallback( just use stream_reader
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PHTAL, thanks. https://codereview.chromium.org/2712713007/diff/80001/content/browser/downloa... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2712713007/diff/80001/content/browser/downloa... content/browser/download/download_file_impl.cc:367: if (state == ByteStreamReader::STREAM_COMPLETE) { On 2017/03/02 00:12:23, qinmin wrote: > do we need to do this if it is just one stream fails? the original place resets > the timer, and it is not done here, Done, removed. The original logic also records bandwidth when stream completes and download is interruptted at the same time, which is kind of wrong. The current way to reset the timer when the stream fails is, it set the reason to interrupted, and tell observer(DownloadItemImpl) which may release the download file and also kill the timer. Multiple streams case also uses the same flow. Probably we should file a bug to dig into it? since the current way is kind of weird. https://codereview.chromium.org/2712713007/diff/80001/content/browser/downloa... content/browser/download/download_file_impl.cc:402: weak_factory_.InvalidateWeakPtrs(); On 2017/03/02 00:12:23, qinmin wrote: > ditto: reset timer here? Done, RecordFileBandwidth and update_timer_ is reset in this block when all streams are complete. The failure case is discussed above. https://codereview.chromium.org/2712713007/diff/80001/content/browser/downloa... content/browser/download/download_file_impl.cc:423: source_stream->stream_reader()->RegisterCallback( On 2017/03/02 00:12:23, qinmin wrote: > just use stream_reader Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nits https://codereview.chromium.org/2712713007/diff/80001/content/browser/downloa... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2712713007/diff/80001/content/browser/downloa... content/browser/download/download_file_impl.cc:76: base::MakeUnique<SourceStream>(save_info_->offset, 0); nit: s/0/DownloadSaveInfo::kLengthFullContent/ https://codereview.chromium.org/2712713007/diff/80001/content/browser/downloa... content/browser/download/download_file_impl.cc:311: if (source_stream->length() > 0 && nit: s/> 0/!= DownloadSaveInfo::kLengthFullContent/
lgtm % nits https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... File content/browser/download/download_file_factory.cc (right): https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_factory.cc:22: std::move(byte_stream), net_log, false, observer); Maybe add /* is_sparse_file */ to false to make it clear? https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.cc:78: source_streams_.begin()->second->SetByteStream(std::move(stream_reader)); source_streams_[save_info_->offset]->SetByteStream()? https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... File content/browser/download/download_file_impl.h (right): https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.h:130: // If all the data read from the stream has been successfully write to disk. write -> written https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.h:221: bool is_sparse_file_; Something to consider, could we rely on source_streams_.size()?
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xingliu@chromium.org changed reviewers: + alexmos@chromium.org
PTAL, thanks. alexmos@chromium.org: Please review changes in content/public/test/test_file_error_injector.cc https://codereview.chromium.org/2712713007/diff/80001/content/browser/downloa... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2712713007/diff/80001/content/browser/downloa... content/browser/download/download_file_impl.cc:76: base::MakeUnique<SourceStream>(save_info_->offset, 0); On 2017/03/02 17:21:34, qinmin wrote: > nit: s/0/DownloadSaveInfo::kLengthFullContent/ Done. https://codereview.chromium.org/2712713007/diff/80001/content/browser/downloa... content/browser/download/download_file_impl.cc:311: if (source_stream->length() > 0 && On 2017/03/02 17:21:34, qinmin wrote: > nit: s/> 0/!= DownloadSaveInfo::kLengthFullContent/ Done. https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... File content/browser/download/download_file_factory.cc (right): https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_factory.cc:22: std::move(byte_stream), net_log, false, observer); On 2017/03/02 18:24:50, David Trainor wrote: > Maybe add /* is_sparse_file */ to false to make it clear? Done. https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.cc:78: source_streams_.begin()->second->SetByteStream(std::move(stream_reader)); On 2017/03/02 18:24:50, David Trainor wrote: > source_streams_[save_info_->offset]->SetByteStream()? Done. https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... File content/browser/download/download_file_impl.h (right): https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.h:130: // If all the data read from the stream has been successfully write to disk. On 2017/03/02 18:24:50, David Trainor wrote: > write -> written Done. https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.h:221: bool is_sparse_file_; On 2017/03/02 18:24:50, David Trainor wrote: > Something to consider, could we rely on source_streams_.size()? Added a TODO here. We will probably pass a slice info vector instead of is_sparse_file in the next CL. Use the size of streams may cause issue if the previous download session is parallel but the resumption is non-parallel, where resumption session will have one stream but write to a sparse file. If the etag is correct, the single stream resumption session can continue to write to the sparse file without deleting it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.cc:45: : offset_(offset), length_(length), bytes_written_(0), finished_(false) {} Is this 'git cl format's doing? https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.cc:125: // |source_streams_| is not thread safe, must be modified on the same thread. This is not safe. The problem here is that the DownloadFileImpl might be getting destroyed or probably already destroyed on the FILE thread. In that case you are indirecting through a freed pointer. The thread specificity of DownloadFileImpl is: * It's created and initialized on the UI thread. * Then it's tossed over to the FILE thread where it lives and dies. If you intend AddByteStream to be called from a thread other than the FILE thread, then: * Make AddByteStream() be static. This way you won't accidentally touch an unsafe member like this code is currently doing. * Make the API contract state explicitly that AddByteStream can only be called by an owner who can guarantee that the destruction of DownloadFileImpl hasn't beed scheduled on the FILE thread. THis is currently only possible if the owner is thread bound and controls the lifetime of the DownloadFileImpl. The latter condition is true for DownloadItemImpl. So you should be safe. * Have a single method on DownloadFileImpl that accepts an incoming ByteStreamReader. Schedule that method using an unretained DownloadFileImpl pointer. Note that WeakPtrFactory isn't thread safe. https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.cc:325: reason = AppendDataToFile(incoming_data.get()->data(), I'm assuming that the plan is to get rid of this logic? https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.cc:389: all_stream_complete = false; This kind of logic tends to be quite fragile. Imagine what would happen if a new ByteStreamReader addition has been scheduled to be added. This logic will cause the DownloadFileImpl to prematurely declare completion before the new stream reader is added. (source_stream_ is also being accessed from both the UI and FILE threads due to this logic here, but that's moot once the other thread safety issues are addressed). https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... File content/browser/download/download_file_impl.h (right): https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.h:48: bool is_sparse_file, The meaning of a bool doesn't directly convey a meaning at the call site of DownloadFileImpl. Something to consider is to use an enum. Compare: DownloadFileImpl(std::move(some_save_info), .., true, observer) vs. DownloadFileImpl(std::move(some_save_info), .., DownloadFile::SPARSE, observer) https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.h:95: // |stream_reader_| can be set later when the network response is handled. Why though? Is there a case where the stream reader is't known at the time SourceStream is created? https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.h:108: // disk. "Called after successfully writing a buffer to disk" it's a given that the read operation was successful. https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.h:127: // Will write to disk at (|offset_| + |bytes_written_|). "Next write position is ..." https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.h:136: // with DownloadFile and get rid of BaseFile. Remove this TODO. Doesn't apply anymore. https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.h:221: bool is_sparse_file_; On 2017/03/02 18:24:50, David Trainor wrote: > Something to consider, could we rely on source_streams_.size()? +1. Also I don't think it makes things any simpler to keep the sparse and contiguous cases separate.
content/public/test/test_file_error_injector.cc LGTM once asanka@'s comments are addressed. https://codereview.chromium.org/2712713007/diff/140001/content/public/test/te... File content/public/test/test_file_error_injector.cc (right): https://codereview.chromium.org/2712713007/diff/140001/content/public/test/te... content/public/test/test_file_error_injector.cc:114: false, nit: consider adding /* is_sparse_file */ for readability, or switching to an enum
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi, thanks for the feedback, please take another look. https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.cc:45: : offset_(offset), length_(length), bytes_written_(0), finished_(false) {} On 2017/03/02 19:37:43, asanka wrote: > Is this 'git cl format's doing? Yes, I think so. https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.cc:125: // |source_streams_| is not thread safe, must be modified on the same thread. On 2017/03/02 19:37:43, asanka wrote: > This is not safe. > > The problem here is that the DownloadFileImpl might be getting destroyed or > probably already destroyed on the FILE thread. In that case you are indirecting > through a freed pointer. > > The thread specificity of DownloadFileImpl is: > * It's created and initialized on the UI thread. > * Then it's tossed over to the FILE thread where it lives and dies. > > If you intend AddByteStream to be called from a thread other than the FILE > thread, then: > * Make AddByteStream() be static. This way you won't accidentally touch an > unsafe member like this code is currently doing. > > * Make the API contract state explicitly that AddByteStream can only be called > by an owner who can guarantee that the destruction of DownloadFileImpl hasn't > beed scheduled on the FILE thread. This is currently only possible if the owner > is thread bound and controls the lifetime of the DownloadFileImpl. The latter > condition is true for DownloadItemImpl. So you should be safe. > > * Have a single method on DownloadFileImpl that accepts an incoming > ByteStreamReader. Schedule that method using an unretained DownloadFileImpl > pointer. > > Note that WeakPtrFactory isn't thread safe. Done, Thanks for this suggestion. Yeah, good catch, DownloadFileImpl's weak_factory_.GetWeakPtr() must be called on file thread here to bind to the file thread task runner token. Then in this way, object on UI thread must make sure download_file unique_ptr is not null when call AddByteStream and use Unretained. For the stream_reader, when we post this function from the UI thread, it's moved and copied to the callback storage so it should also be ok. For source_streams_(unordered_map), since we don't change its size but only pass the parameters from callback storage it should be fine. PTAL at the new code here. https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.cc:325: reason = AppendDataToFile(incoming_data.get()->data(), On 2017/03/02 19:37:43, asanka wrote: > I'm assuming that the plan is to get rid of this logic? Done. https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.cc:389: all_stream_complete = false; On 2017/03/02 19:37:43, asanka wrote: > This kind of logic tends to be quite fragile. Imagine what would happen if a new > ByteStreamReader addition has been scheduled to be added. This logic will cause > the DownloadFileImpl to prematurely declare completion before the new stream > reader is added. > > (source_stream_ is also being accessed from both the UI and FILE threads due to > this logic here, but that's moot once the other thread safety issues are > addressed). Done, Added some comment in the header. Already noticed this issue, SourceStreams is not supposed to dynamically add or remove elements after the ctor(UI thread). https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... File content/browser/download/download_file_impl.h (right): https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.h:48: bool is_sparse_file, On 2017/03/02 19:37:43, asanka wrote: > The meaning of a bool doesn't directly convey a meaning at the call site of > DownloadFileImpl. Something to consider is to use an enum. Compare: > > DownloadFileImpl(std::move(some_save_info), .., true, observer) > > vs. > > DownloadFileImpl(std::move(some_save_info), .., DownloadFile::SPARSE, > observer) > This will change soon in next CL that we will pass slice info vector instead of a bool. Have put a comment in header on |is_sparse_file_| for this. I remember we used to have an enum is BaseFile but later convert to use bool for simplicity. https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.h:95: // |stream_reader_| can be set later when the network response is handled. On 2017/03/02 19:37:43, asanka wrote: > Why though? Is there a case where the stream reader is't known at the time > SourceStream is created? Yes, the byte stream will come later for parallel requests. Only the first byte stream is ready in ctor of DownloadFileImpl. https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.h:108: // disk. On 2017/03/02 19:37:43, asanka wrote: > "Called after successfully writing a buffer to disk" > > it's a given that the read operation was successful. Done. https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.h:127: // Will write to disk at (|offset_| + |bytes_written_|). On 2017/03/02 19:37:43, asanka wrote: > "Next write position is ..." Done. https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.h:136: // with DownloadFile and get rid of BaseFile. On 2017/03/02 19:37:43, asanka wrote: > Remove this TODO. Doesn't apply anymore. Done. https://codereview.chromium.org/2712713007/diff/100001/content/browser/downlo... content/browser/download/download_file_impl.h:221: bool is_sparse_file_; On 2017/03/02 19:37:43, asanka wrote: > On 2017/03/02 18:24:50, David Trainor wrote: > > Something to consider, could we rely on source_streams_.size()? > > +1. Also I don't think it makes things any simpler to keep the sparse and > contiguous cases separate. Added a TODO that we will use slice info later. https://codereview.chromium.org/2712713007/diff/140001/content/public/test/te... File content/public/test/test_file_error_injector.cc (right): https://codereview.chromium.org/2712713007/diff/140001/content/public/test/te... content/public/test/test_file_error_injector.cc:114: false, On 2017/03/02 21:45:14, alexmos wrote: > nit: consider adding /* is_sparse_file */ for readability, or switching to an > enum Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM thanks!
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xingliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, qinmin@chromium.org, alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/2712713007/#ps170001 (title: "Remove the AppendDataToFile call, fix the browsertest.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 170001, "attempt_start_ts": 1488583370710760,
"parent_rev": "e962df5122998010c9db55d0fb46993f058e7db4", "commit_rev":
"992dacf666e6750a1911caebe43b29c93ea4d65f"}
Message was sent while issue was closed.
Description was changed from ========== Make DownloadFileImpl handle multiple byte streams. DownloadFileImpl now hold multiple byte streams, add a wrapper class for each stream reader. Didn't hook to slice info or parallel job. BUG=644352 ========== to ========== Make DownloadFileImpl handle multiple byte streams. DownloadFileImpl now hold multiple byte streams, add a wrapper class for each stream reader. Didn't hook to slice info or parallel job. BUG=644352 Review-Url: https://codereview.chromium.org/2712713007 Cr-Commit-Position: refs/heads/master@{#454709} Committed: https://chromium.googlesource.com/chromium/src/+/992dacf666e6750a1911caebe43b... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:170001) as https://chromium.googlesource.com/chromium/src/+/992dacf666e6750a1911caebe43b... |
