[Download Service] Downloads are limited to 15 even if they completed |
||
Issue descriptionIf I create a DownloadService::Client, I find I can only download 15 files before I start receiving BACKOFF. This happens even if the download completes successfully. It looks to be because of https://cs.chromium.org/chromium/src/components/download/internal/controller_impl.cc?sq=package:chromium&dr=CSs&l=148 Here the client_count that is checked and if it's >= the max (15) then it sends BACKOFF. client_count is dependent on the entries which only ever decrease if ModelImpl::Remove is called. This only happens for these methods in ControllerImpl: RemoveCleanupEligibleDownloads CancelOrphanedRequests HandleStartDownloadResponse HandleCompleteDownload The HandleCompleteDownload case doesn't remove the entry if the download was successful, presumably so it can delete the file once the timeout has triggered. I think the problem really is that GetNumberOfEntriesForClient is being used which returns the number of entries rather than getting the number of _active_ entries. Current WIP CL implementing BackgroundFetch using the DownloadService https://chromium-review.googlesource.com/c/598027
,
Aug 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1ae822ba688d46ceb251a741f3aa2bbf7bdb54f6 commit 1ae822ba688d46ceb251a741f3aa2bbf7bdb54f6 Author: David Trainor <dtrainor@chromium.org> Date: Thu Aug 17 21:55:58 2017 Fix download service BACKOFF count bug The DownloadService was counting completed downloads as part of it's backoff handling. This was causing unexpected BACKOFF responses to new queued downloads because completed downloads were bucketted towards the count. Completed downloads are kept around for a period of around 12 hours to make sure the downloaded file on disk gets properly cleaned up, but the download should be effectively invisible otherwise. The fix is to ignore completed downloads when counting downloads associated with a particular client. BUG= 755119 Change-Id: Ic5cc489885c6eeec56e04955f5009b56980c8bfb Reviewed-on: https://chromium-review.googlesource.com/614802 Reviewed-by: Shakti Sahu <shaktisahu@chromium.org> Reviewed-by: Xing Liu <xingliu@chromium.org> Commit-Queue: David Trainor <dtrainor@chromium.org> Cr-Commit-Position: refs/heads/master@{#495335} [modify] https://crrev.com/1ae822ba688d46ceb251a741f3aa2bbf7bdb54f6/components/download/internal/controller_impl.cc [modify] https://crrev.com/1ae822ba688d46ceb251a741f3aa2bbf7bdb54f6/components/download/internal/entry_utils.cc [modify] https://crrev.com/1ae822ba688d46ceb251a741f3aa2bbf7bdb54f6/components/download/internal/entry_utils.h [modify] https://crrev.com/1ae822ba688d46ceb251a741f3aa2bbf7bdb54f6/components/download/internal/entry_utils_unittest.cc
,
Aug 17 2017
Just realized I did not add a test to protect against regressions here. Will add that in a follow up patch.
,
Nov 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3ce8989fc4ed6eabdb1a4e3c02ed3e688749fcbe commit 3ce8989fc4ed6eabdb1a4e3c02ed3e688749fcbe Author: Dan Elphick <delphick@chromium.org> Date: Thu Nov 09 17:21:21 2017 Remove stale TODO in BackgroundFetchDelegateImpl Now that crbug.com/755119 has been fixed, remove TODO and add a NOTREACHED for BACKOFF behaviour which we don't currently support. In reality the NOTREACHED is very unlikely to trigger as you'd need to launch 15+ background fetches simultaneously, which is very unlikely for a flagged feature. Bug: 755119 Change-Id: Ic70d056486e695e7953a8c58515eca31c1d3f935 Reviewed-on: https://chromium-review.googlesource.com/758781 Reviewed-by: Peter Beverloo <peter@chromium.org> Commit-Queue: Peter Beverloo <peter@chromium.org> Cr-Commit-Position: refs/heads/master@{#515196} [modify] https://crrev.com/3ce8989fc4ed6eabdb1a4e3c02ed3e688749fcbe/chrome/browser/background_fetch/background_fetch_delegate_impl.cc
,
Jun 7 2018
|
||
►
Sign in to add a comment |
||
Comment 1 by dtrainor@chromium.org
, Aug 14 2017