Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Closes #9354 improve resume downloads in slow networks #9036

Merged
merged 1 commit into from
Nov 30, 2020

Conversation

Amejia481
Copy link
Contributor

In cases where users with slow networks start a download but quickly press pause and then resume, there will be not enough time for bytes to be copied, but the stream in [download.response] will be closed, and when the user will retry we will try to use [download.response] over and over with a closed stream as a result the download will failed every time, we have to fallback to httpClient

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

@Amejia481 Amejia481 added the 🕵️‍♀️ needs review PRs that need to be reviewed label Nov 21, 2020
@Amejia481 Amejia481 requested a review from csadilek as a code owner November 21, 2020 01:56
@csadilek csadilek self-assigned this Nov 26, 2020
Copy link
Contributor

@csadilek csadilek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I found one thing below I think we should improve before landing.

// networks start a download but quickly press pause and then resume
// [isResumingDownload] will be false as there will be not enough time
// for bytes to be copied, but the stream in [download.response] will be closed,
// we have to fallback to [httpClient]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this logic would also be applied for downloads already using the HTTP client. I think in this case we don't need to fall back and we don't need to call performDownload again as we're already using the same client anyway. I think we should check for that here...only fall back to the fetch client if we're not already using it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think it's worth filing this for GV to potentially improve the API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this logic would also be applied for downloads already using the HTTP client. I think in this case we don't need to fall back and we don't need to call performDownload again as we're already using the same client anyway. I think we should check for that here...only fall back to the fetch client if we're not already using it.

👍🏽

I also think it's worth filing this for GV to potentially improve the API.

👍🏽

Copy link
Contributor

@csadilek csadilek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the commit references the wrong ticket number, should be #9033.

@Amejia481
Copy link
Contributor Author

Looks like the commit references the wrong ticket number, should be #9033.

Thanks, I will update it with the correct one :)

Copy link
Contributor

@csadilek csadilek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 🚢

If you have a GV ticket, I would link it in your comment above where you describe why we're doing this.

@Amejia481
Copy link
Contributor Author

@Amejia481 Amejia481 added 🛬 needs landing PRs that are ready to land and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Nov 30, 2020
@mergify mergify bot merged commit 9ec6a29 into mozilla-mobile:master Nov 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🛬 needs landing PRs that are ready to land
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants