Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2738063002:
Verify that all slice are downloaded when a stream complete (Closed)
DescriptionVerify that all slice are downloaded when a stream complete
To determine if a download finishes, we check if all streams are completed.
However, some of the byte streams might not have been added to DownloadFileImpl.
As a result, this CL checks if all the slices are written.
BUG=644352
Review-Url: https://codereview.chromium.org/2738063002
Cr-Commit-Position: refs/heads/master@{#456903}
Committed: https://chromium.googlesource.com/chromium/src/+/9c8aefe72593bb63ccc20dd34fae9ea034b3d709
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix bug #
Total comments: 7
Patch Set 3 : rebase #
Messages
Total messages: 22 (9 generated)
qinmin@chromium.org changed reviewers: + dtrainor@chromium.org, xingliu@chromium.org
PTAL
https://codereview.chromium.org/2738063002/diff/1/content/browser/download/do... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2738063002/diff/1/content/browser/download/do... content/browser/download/download_file_impl.cc:429: int64_t last_slice_offset = 0; You need to set this inside the if block below right? Or just use steam_for_last_slice->offset()?
https://codereview.chromium.org/2738063002/diff/1/content/browser/download/do... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2738063002/diff/1/content/browser/download/do... content/browser/download/download_file_impl.cc:429: int64_t last_slice_offset = 0; On 2017/03/09 17:23:10, David Trainor-ping if over 24h wrote: > You need to set this inside the if block below right? Or just use > steam_for_last_slice->offset()? Good catch. I was using steam_for_last_slice = source_stream.begin()->second to initialize stream_for_last_slice. But that's not correct since all the source_streams may have written 0 bytes. So I introduced this variable, but forgot to update the for loop to update this variable. Fixed.
https://codereview.chromium.org/2738063002/diff/20001/content/browser/downloa... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2738063002/diff/20001/content/browser/downloa... content/browser/download/download_file_impl.cc:445: std::vector<DownloadItem::ReceivedSlice> slices_to_download = So if we do this, source_streams_ don't need to be fully setup in the ctor, but can add/remove element when AddByteStream is called? This is probably better.
https://codereview.chromium.org/2738063002/diff/20001/content/browser/downloa... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2738063002/diff/20001/content/browser/downloa... content/browser/download/download_file_impl.cc:445: std::vector<DownloadItem::ReceivedSlice> slices_to_download = On 2017/03/09 21:24:23, xingliu wrote: > So if we do this, source_streams_ don't need to be fully setup in the ctor, but > can add/remove element when AddByteStream is called? This is probably better. The only element in the source_streams_ that needs to be setup is the initial request. All other SourceStreams can be constructed later when needed.
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: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm with nits%. Some thoughts on the criteria to check the last slice. I think the current logic involves some risk. https://codereview.chromium.org/2738063002/diff/20001/content/browser/downloa... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2738063002/diff/20001/content/browser/downloa... content/browser/download/download_file_impl.cc:451: } else if (stream_for_last_slice) { nit%: use if instead of else if, since previous block has returned. Also this block check if the last source stream is half opened. This probably based on an assumption that we will always send a half opened request even the last slice is finished, right? This involves some risk since even if the last slice is done, we send a half opened request which doesn't download anything. Also, maybe we should consider to use content-length to check if the final slice is done, maybe better than send a request for whatever reason, wdyt? https://codereview.chromium.org/2738063002/diff/20001/content/browser/downloa... content/browser/download/download_file_impl.cc:454: // The last stream should not have a length limit. If it has, it might nit%: The first stream also doesn't have length limit, do we want the first stream to be able to cover other slices? If so stream_for_last_slice->length() may not be 0 when first stream is done, but the function will return false.
For the failing tests, this CL depends on https://codereview.chromium.org/2737033002/ to fix them. As that change will introduce the logic update the |received_slices| vector. https://codereview.chromium.org/2738063002/diff/20001/content/browser/downloa... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2738063002/diff/20001/content/browser/downloa... content/browser/download/download_file_impl.cc:451: } else if (stream_for_last_slice) { On 2017/03/10 00:46:22, xingliu wrote: > nit%: use if instead of else if, since previous block has returned. > > Also this block check if the last source stream is half opened. This probably > based on an assumption that we will always send a half opened request even the > last slice is finished, right? > > This involves some risk since even if the last slice is done, we send a half > opened request which doesn't download anything. > > Also, maybe we should consider to use content-length to check if the final slice > is done, maybe better than send a request for whatever reason, wdyt? Done. The issue is that using the content-length check is not safe. And if the downloaded content is already longer than content-length, we will still face the same problem. The safest approach is to add a boolean variable into the history db, or we do a special request like this. https://codereview.chromium.org/2738063002/diff/20001/content/browser/downloa... content/browser/download/download_file_impl.cc:454: // The last stream should not have a length limit. If it has, it might On 2017/03/10 00:46:22, xingliu wrote: > nit%: The first stream also doesn't have length limit, do we want the first > stream to be able to cover other slices? > > If so stream_for_last_slice->length() may not be 0 when first stream is done, > but the function will return false. I don't think the situation you mentioned will happen. Since stream_for_last_slice's bytes_written() > 0, it means that the stream was able to write a chunk of data to the file. Any stream with offset smaller than stream_for_last_slice should have stopped short. Or otherwise the same chunk was written by more than 2 workers and violates the received_slices integrity check. If the first stream is able to reach the end, it means no other stream has written any data to the file. Or otherwise it will be stopped short by another stream. As a result, stream_for_last_slice should be the first stream
If parallel download unit tests are still broken, we probably can disable them at the moment. https://codereview.chromium.org/2738063002/diff/20001/content/browser/downloa... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2738063002/diff/20001/content/browser/downloa... content/browser/download/download_file_impl.cc:451: } else if (stream_for_last_slice) { On 2017/03/10 07:11:27, qinmin wrote: > On 2017/03/10 00:46:22, xingliu wrote: > > nit%: use if instead of else if, since previous block has returned. > > > > Also this block check if the last source stream is half opened. This probably > > based on an assumption that we will always send a half opened request even the > > last slice is finished, right? > > > > This involves some risk since even if the last slice is done, we send a half > > opened request which doesn't download anything. > > > > Also, maybe we should consider to use content-length to check if the final > slice > > is done, maybe better than send a request for whatever reason, wdyt? > > Done. > > The issue is that using the content-length check is not safe. And if the > downloaded content is already longer than content-length, we will still face the > same problem. The safest approach is to add a boolean variable into the history > db, or we do a special request like this. Yeah, make sense. For special request, I doubt some server may give different response code.
On 2017/03/10 18:17:03, xingliu wrote: > If parallel download unit tests are still broken, we probably can disable them > at the moment. > > https://codereview.chromium.org/2738063002/diff/20001/content/browser/downloa... > File content/browser/download/download_file_impl.cc (right): > > https://codereview.chromium.org/2738063002/diff/20001/content/browser/downloa... > content/browser/download/download_file_impl.cc:451: } else if > (stream_for_last_slice) { > On 2017/03/10 07:11:27, qinmin wrote: > > On 2017/03/10 00:46:22, xingliu wrote: > > > nit%: use if instead of else if, since previous block has returned. > > > > > > Also this block check if the last source stream is half opened. This > probably > > > based on an assumption that we will always send a half opened request even > the > > > last slice is finished, right? > > > > > > This involves some risk since even if the last slice is done, we send a > half > > > opened request which doesn't download anything. > > > > > > Also, maybe we should consider to use content-length to check if the final > > slice > > > is done, maybe better than send a request for whatever reason, wdyt? > > > > Done. > > > > The issue is that using the content-length check is not safe. And if the > > downloaded content is already longer than content-length, we will still face > the > > same problem. The safest approach is to add a boolean variable into the > history > > db, or we do a special request like this. > > Yeah, make sense. > > For special request, I doubt some server may give different response code. We need to test what the server response when this happens. Or an alternative is to issue a request at half open request: content-length -1 , and then inform the DownloadFile about this special handling
On 2017/03/10 18:28:48, qinmin wrote: > On 2017/03/10 18:17:03, xingliu wrote: > > If parallel download unit tests are still broken, we probably can disable them > > at the moment. > > > > > https://codereview.chromium.org/2738063002/diff/20001/content/browser/downloa... > > File content/browser/download/download_file_impl.cc (right): > > > > > https://codereview.chromium.org/2738063002/diff/20001/content/browser/downloa... > > content/browser/download/download_file_impl.cc:451: } else if > > (stream_for_last_slice) { > > On 2017/03/10 07:11:27, qinmin wrote: > > > On 2017/03/10 00:46:22, xingliu wrote: > > > > nit%: use if instead of else if, since previous block has returned. > > > > > > > > Also this block check if the last source stream is half opened. This > > probably > > > > based on an assumption that we will always send a half opened request even > > the > > > > last slice is finished, right? > > > > > > > > This involves some risk since even if the last slice is done, we send a > > half > > > > opened request which doesn't download anything. > > > > > > > > Also, maybe we should consider to use content-length to check if the final > > > slice > > > > is done, maybe better than send a request for whatever reason, wdyt? > > > > > > Done. > > > > > > The issue is that using the content-length check is not safe. And if the > > > downloaded content is already longer than content-length, we will still face > > the > > > same problem. The safest approach is to add a boolean variable into the > > history > > > db, or we do a special request like this. > > > > Yeah, make sense. > > > > For special request, I doubt some server may give different response code. > > We need to test what the server response when this happens. Or an alternative is > to issue a request at half open request: content-length -1 , and then inform the > DownloadFile about this special handling issue a half open request from content-length -1
lgtm
The CQ bit was checked by qinmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, xingliu@chromium.org Link to the patchset: https://codereview.chromium.org/2738063002/#ps40001 (title: "rebase")
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": 40001, "attempt_start_ts": 1489530450646580,
"parent_rev": "2f676ea4689dec8df2f636d7d768ea752eac8113", "commit_rev":
"9c8aefe72593bb63ccc20dd34fae9ea034b3d709"}
Message was sent while issue was closed.
Description was changed from ========== Verify that all slice are downloaded when a stream complete To determine if a download finishes, we check if all streams are completed. However, some of the byte streams might not have been added to DownloadFileImpl. As a result, this CL checks if all the slices are written. BUG=644352 ========== to ========== Verify that all slice are downloaded when a stream complete To determine if a download finishes, we check if all streams are completed. However, some of the byte streams might not have been added to DownloadFileImpl. As a result, this CL checks if all the slices are written. BUG=644352 Review-Url: https://codereview.chromium.org/2738063002 Cr-Commit-Position: refs/heads/master@{#456903} Committed: https://chromium.googlesource.com/chromium/src/+/9c8aefe72593bb63ccc20dd34fae... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/9c8aefe72593bb63ccc20dd34fae... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
