-
-
Notifications
You must be signed in to change notification settings - Fork 140
Support automatic resumption of interrupted downloads #173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Owner
|
I don't think |
Owner
|
AI review: Blockers
API/Docs mismatches
Implementation fixes (minimal)diff --git a/index.js b/index.js
@@
-const listener = (event, item, webContents) => {
+const listener = (event, item, webContents) => {
+ // Safe defaults even when `options` is undefined:
+ const {
+ retryLimit = 60,
+ // Drop this entirely unless you decide to document it:
+ toleranceBytes = 0
+ } = options ?? {};
let currentRetry = 0;
- let itemLastTransferredBytes = 0;
+ let itemLastTransferredBytes = 0;
@@
- item.on('updated', (event, state) => {
- const retryLimit = options.retryLimit ?? 60;
- const toleranceBytes = options.toleranceBytes ?? 1024 * 50;
+ item.on('updated', (event, state) => {
receivedBytes = completedBytes;
@@
- const itemTransferredBytes = item.getReceivedBytes();
- const itemTotalBytes = item.getTotalBytes();
- const hasTransferredBytesChanged = itemTransferredBytes > itemLastTransferredBytes + toleranceBytes;
- itemLastTransferredBytes = itemTransferredBytes;
- if (hasTransferredBytesChanged) {
- currentRetry = 0;
- }
+ const itemTransferredBytes = item.getReceivedBytes();
+ const itemTotalBytes = item.getTotalBytes();
+ // Simpler & correct: reset on any real progress during 'progressing'
+ if (state === 'progressing' && itemTransferredBytes > itemLastTransferredBytes + toleranceBytes) {
+ itemLastTransferredBytes = itemTransferredBytes;
+ currentRetry = 0;
+ }
@@
- if (state === 'interrupted') {
- if (item.canResume() && currentRetry < retryLimit) {
+ if (state === 'interrupted') {
+ if (item.canResume() && currentRetry < retryLimit) {
currentRetry++;
setTimeout(() => { item.resume(); }, 1000);
- } else {
- const message = pupa(errorMessage, {filename: path.basename(filePath)});
- callback(new Error(message));
- item.cancel();
- }
+ } else {
+ // Preserve existing UX: surface the “interrupted” error instead of converting to “cancelled”.
+ const message = pupa(errorMessage, {filename: path.basename(filePath)});
+ // If running via download(...), reject; if registered globally, show the dialog.
+ if (callback !== noop) { // use the actual sentinel you already pass
+ callback(new Error(message));
+ // Do NOT cancel here; let 'done' emit 'interrupted' per Electron semantics.
+ } else {
+ // Global registered mode shows dialog per README; ensure that still happens.
+ }
+ }
}
});And make the resume scheduling single-shot: - setTimeout(() => { item.resume(); }, 1000);
+ if (!retryTimer) {
+ retryTimer = setTimeout(() => { retryTimer = undefined; item.resume(); }, 1000);
+ }Types/Docs
// existing Options…
/** Number of 1s resume attempts after an interruption. @default 60 */
readonly retryLimit?: number;
/** Bytes of confirmed forward progress required to reset the retry counter. @default 0 */
readonly toleranceBytes?: number; // or drop it
Tests
Misc
Bottom line: keep the feature, fix the callback/cancel semantics, remove or document |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Support automatic resumption of interrupted downloads
Summary
This pull request improves the handling of the
interrupteddownload state inelectron-dl. Currently, when a download is interrupted and the'updated'event is emitted, there is no automatic attempt to resume the download.This PR includes:
item.canResume()returns true.retryLimitparameter (default: 60 attempts).Motivation
According to the official Electron documentation, interrupted downloads can typically be resumed within the
'updated'event by usingitem.resume().There was a previous attempt to address this in PR #71, but it was not merged at the time.
Without this improvement, interrupted downloads may fail permanently (only on Windows) even if they are resumable. This PR improves download stability by automatically retrying interrupted downloads that can be resumed, helping to recover from temporary network problems.
Example Usage