From 4b09a0db6702c59d3b97c69bd9d32bc7772ba35a Mon Sep 17 00:00:00 2001 From: Nils Maier Date: Thu, 12 Sep 2019 20:51:37 +0200 Subject: [PATCH] Retries Closes #95 --- TODO.md | 4 -- _locales/en/messages.json | 22 ++++++++++ data/prefs.json | 2 + lib/db.ts | 15 +++++-- lib/manager/basedownload.ts | 8 +++- lib/manager/download.ts | 64 ++++++++++++++++++++++++---- lib/manager/man.ts | 83 ++++++++++++++++++++++++++++++------- lib/manager/state.ts | 9 ++-- style/common.css | 1 + style/manager.css | 17 ++++++++ style/prefs.css | 7 ++++ windows/manager/state.ts | 3 ++ windows/manager/table.ts | 8 ++++ windows/prefs.html | 9 +++- windows/prefs.ts | 2 + 15 files changed, 215 insertions(+), 39 deletions(-) diff --git a/TODO.md b/TODO.md index 3544a47..b4f9798 100644 --- a/TODO.md +++ b/TODO.md @@ -8,8 +8,6 @@ P2 Planned for later. -* Soft errors and retry logic - * Big caveat: When the server still responds, like 50x errors which would be recoverable, we actually have no way of knowing it did in respond in such a way. See P4 - Handle Errors remarks. * Inter-addon API (basic) * Add downloads * vtable perf: cache column widths @@ -44,8 +42,6 @@ Stuff that probably cannot be implemented due to WeberEension limitations. * Firefox helpfully keeps different lists of downloads. One for newly added downloads, and other ones for "previous" downloads. Turns out the WebExtension API only ever queries the "new" list. * Segmented downloads * Cannot be done with WebExtensions - downloads API has no support and manually downloading, storing in temporary add-on storage and reassmbling the downloaded parts later is not only efficient but does not reliabliy work due to storage limitations. -* Handle errors, 404 and such - * The Firefox download manager is too stupid and webRequest does not see Downloads, so cannot be done right now. * Conflicts: ask when a file exists * Not supported by Firefox * Speed limiter diff --git a/_locales/en/messages.json b/_locales/en/messages.json index 0cedd36..e50e9e4 100644 --- a/_locales/en/messages.json +++ b/_locales/en/messages.json @@ -617,6 +617,14 @@ "description": "Preferences/General", "message": "Remove missing downloads after a restart" }, + "pref_retries": { + "description": "pref text", + "message": "Number of retries of downloads on temporary errors" + }, + "pref_retry_time": { + "description": "pref text", + "message": "Retry every (in minutes)" + }, "pref_show_urls": { "description": "Preferences/General", "message": "Show URLs instead of Names" @@ -955,6 +963,20 @@ "description": "Action for resuming a download", "message": "Resume" }, + "retrying": { + "description": "Status text", + "message": "Retrying" + }, + "retrying_error": { + "description": "status text", + "message": "Retrying - $ERROR$", + "placeholders": { + "error": { + "content": "$1", + "example": "Server Error" + } + } + }, "running": { "description": "Status text", "message": "Running" diff --git a/data/prefs.json b/data/prefs.json index bebe9e0..a164d1e 100644 --- a/data/prefs.json +++ b/data/prefs.json @@ -15,6 +15,8 @@ "tooltip": true, "show-urls": false, "remove-missing-on-init": false, + "retries": 5, + "retry-time": 10, "limits": [ { "domain": "*", diff --git a/lib/db.ts b/lib/db.ts index 2fe8f59..48db4ba 100644 --- a/lib/db.ts +++ b/lib/db.ts @@ -2,6 +2,9 @@ // eslint-disable-next-line no-unused-vars import { BaseItem } from "./item"; +// eslint-disable-next-line no-unused-vars +import { Download } from "./manager/download"; +import { RUNNING, QUEUED, RETRYING } from "./manager/state"; // License: MIT @@ -69,7 +72,7 @@ export const DB = new class DB { return await new Promise(this.getAllInternal); } - saveItemsInternal(items: any[], resolve: Function, reject: Function) { + saveItemsInternal(items: Download[], resolve: Function, reject: Function) { if (!items || !items.length || !this.db) { resolve(); return; @@ -83,9 +86,13 @@ export const DB = new class DB { if (item.private) { continue; } - const req = store.put(item.toJSON()); + const json = item.toJSON(); + if (item.state === RUNNING || item.state === RETRYING) { + json.state = QUEUED; + } + const req = store.put(json); if (!("dbId" in item) || item.dbId < 0) { - req.onsuccess = () => item.dbId = req.result; + req.onsuccess = () => item.dbId = req.result as number; } } } @@ -94,7 +101,7 @@ export const DB = new class DB { } } - async saveItems(items: any[]) { + async saveItems(items: Download[]) { await this.init(); return await new Promise(this.saveItemsInternal.bind(this, items)); } diff --git a/lib/manager/basedownload.ts b/lib/manager/basedownload.ts index 7ceaec8..1618c3b 100644 --- a/lib/manager/basedownload.ts +++ b/lib/manager/basedownload.ts @@ -48,7 +48,9 @@ const DEFAULTS = { written: 0, manId: 0, mime: "", - prerolled: false + prerolled: false, + retries: 0, + deadline: 0 }; let sessionId = 0; @@ -105,6 +107,8 @@ export class BaseDownload { public prerolled: boolean; + public retries: number; + constructor(options: any) { Object.assign(this, DEFAULTS); this.assign(options); @@ -113,6 +117,7 @@ export class BaseDownload { } this.sessionId = ++sessionId; this.renamer = new Renamer(this); + this.retries = 0; } assign(options: any) { @@ -182,6 +187,7 @@ export class BaseDownload { rv.currentName = this.browserName || rv.destName || rv.finalName; rv.error = this.error; rv.ext = this.renamer.p_ext; + rv.retries = this.retries; return rv; } } diff --git a/lib/manager/download.ts b/lib/manager/download.ts index 1b56062..348baaa 100644 --- a/lib/manager/download.ts +++ b/lib/manager/download.ts @@ -3,7 +3,7 @@ // eslint-disable-next-line no-unused-vars import { CHROME, downloads, DownloadOptions } from "../browser"; -import { Prefs } from "../prefs"; +import { Prefs, PrefWatcher } from "../prefs"; import { PromiseSerializer } from "../pserializer"; import { filterInSitu, parsePath } from "../util"; import { BaseDownload } from "./basedownload"; @@ -19,10 +19,27 @@ import { PAUSABLE, PAUSED, QUEUED, - RUNNING + RUNNING, + RETRYING } from "./state"; import { Preroller } from "./preroller"; +function isRecoverable(error: string) { + switch (error) { + case "CRASH": + return true; + + case "SERVER_FAILED": + return true; + + default: + return error.startsWith("NETWORK_"); + } +} + +const RETRIES = new PrefWatcher("retries", 5); +const RETRY_TIME = new PrefWatcher("retry-time", 5); + export class Download extends BaseDownload { public manager: Manager; @@ -34,6 +51,10 @@ export class Download extends BaseDownload { public error: string; + public dbId: number; + + public deadline: number; + constructor(manager: Manager, options: any) { super(options); this.manager = manager; @@ -86,6 +107,7 @@ export class Download extends BaseDownload { return; } catch (ex) { + console.error("cannot resume", ex); this.manager.removeManId(this.manId); this.removeFromBrowser(); } @@ -182,8 +204,7 @@ export class Download extends BaseDownload { this.serverName = res.name; } if (res.error) { - this.cancel(); - this.error = res.error; + this.cancelAccordingToError(res.error); } } catch (ex) { @@ -209,20 +230,32 @@ export class Download extends BaseDownload { } } - async pause() { + async pause(retry?: boolean) { if (!(PAUSABLE & this.state)) { return; } + + if (!retry) { + this.retries = 0; + this.deadline = 0; + } + else { + // eslint-disable-next-line no-magic-numbers + this.deadline = Date.now() + RETRY_TIME.value * 60 * 1000; + } + if (this.state === RUNNING && this.manId) { try { await downloads.pause(this.manId); } catch (ex) { console.error("pause", ex.toString(), ex); + this.cancel(); return; } } - this.changeState(PAUSED); + + this.changeState(retry ? RETRYING : PAUSED); } reset() { @@ -230,6 +263,8 @@ export class Download extends BaseDownload { this.manId = 0; this.written = this.totalSize = 0; this.mime = this.serverName = this.browserName = ""; + this.retries = 0; + this.deadline = 0; } async removeFromBrowser() { @@ -262,6 +297,17 @@ export class Download extends BaseDownload { this.changeState(CANCELED); } + async cancelAccordingToError(error: string) { + if (!isRecoverable(error) || ++this.retries > RETRIES.value) { + this.cancel(); + this.error = error; + return; + } + + await this.pause(true); + this.error = error; + } + setMissing() { if (this.manId) { this.manager.removeManId(this.manId); @@ -318,8 +364,7 @@ export class Download extends BaseDownload { this.changeState(PAUSED); } else if (error) { - this.cancel(); - this.error = error; + this.cancelAccordingToError(error); } else { this.changeState(RUNNING); @@ -330,6 +375,9 @@ export class Download extends BaseDownload { if (state.paused) { this.changeState(PAUSED); } + else if (error) { + this.cancelAccordingToError(error); + } else { this.cancel(); this.error = error || ""; diff --git a/lib/manager/man.ts b/lib/manager/man.ts index 95a9fbd..65eaeb7 100644 --- a/lib/manager/man.ts +++ b/lib/manager/man.ts @@ -4,11 +4,11 @@ import { EventEmitter } from "../events"; import { Notification } from "../notifications"; import { DB } from "../db"; -import { QUEUED, CANCELED, RUNNING } from "./state"; +import { QUEUED, CANCELED, RUNNING, RETRYING } from "./state"; // eslint-disable-next-line no-unused-vars import { Bus, Port } from "../bus"; import { sort } from "../sorting"; -import { Prefs } from "../prefs"; +import { Prefs, PrefWatcher } from "../prefs"; import { _ } from "../i18n"; import { CoalescedUpdate, mapFilterInSitu, filterInSitu } from "../util"; import { PromiseSerializer } from "../pserializer"; @@ -30,6 +30,9 @@ const setShelfEnabled = downloads.setShelfEnabled || function() { // ignored }; +const FINISH_NOTIFICATION = new PrefWatcher("finish-notification", true); +const SOUNDS = new PrefWatcher("sounds", false); + export class Manager extends EventEmitter { private items: Download[]; @@ -49,10 +52,14 @@ export class Manager extends EventEmitter { private readonly running: Set; + private readonly retrying: Set; + private scheduler: Scheduler | null; private shouldReload: boolean; + private deadlineTimer: number; + constructor() { super(); this.active = true; @@ -63,11 +70,13 @@ export class Manager extends EventEmitter { AUTOSAVE_TIMEOUT, this.save.bind(this)); this.dirty = new CoalescedUpdate( DIRTY_TIMEOUT, this.processDirty.bind(this)); + this.processDeadlines = this.processDeadlines.bind(this); this.sids = new Map(); this.manIds = new Map(); this.ports = new Set(); this.scheduler = null; this.running = new Set(); + this.retrying = new Set(); this.startNext = PromiseSerializer.wrapNew(1, this, this.startNext); @@ -188,14 +197,11 @@ export class Manager extends EventEmitter { this.notifiedFinished = false; } - async maybeRunFinishActions() { - if (this.running.size) { - return; - } - await this.maybeNotifyFinished(); + maybeRunFinishActions() { if (this.running.size) { return; } + this.maybeNotifyFinished(); if (this.shouldReload) { this.saveQueue.trigger(); setTimeout(() => { @@ -208,20 +214,15 @@ export class Manager extends EventEmitter { setShelfEnabled(true); } - async maybeNotifyFinished() { - if (this.notifiedFinished || this.running.size) { + maybeNotifyFinished() { + if (this.notifiedFinished || this.running.size || this.retrying.size) { return; } - const notification = await Prefs.get("finish-notification", true); - const sounds = await Prefs.get("sounds", false); - if (this.notifiedFinished || this.running.size) { - return; - } - if (sounds) { + if (SOUNDS.value) { const audio = new Audio(runtime.getURL("/style/done.opus")); audio.addEventListener("canplaythrough", () => audio.play()); } - if (notification) { + if (FINISH_NOTIFICATION.value) { new Notification(null, _("queue-finished")); } this.notifiedFinished = true; @@ -323,6 +324,10 @@ export class Manager extends EventEmitter { if (oldState === RUNNING) { this.running.delete(download); } + else if (oldState === RETRYING) { + this.retrying.delete(download); + this.findDeadline(); + } if (newState === QUEUED) { this.resetScheduler(); this.startNext().catch(console.error); @@ -334,10 +339,56 @@ export class Manager extends EventEmitter { this.running.add(download); } else { + if (newState === RETRYING) { + this.addRetry(download); + } this.startNext().catch(console.error); } } + addRetry(download: Download) { + this.retrying.add(download); + this.findDeadline(); + } + + private findDeadline() { + let deadline = Array.from(this.retrying). + reduce((deadline, item) => { + if (deadline) { + return item.deadline ? Math.min(deadline, item.deadline) : deadline; + } + return item.deadline; + }, 0); + if (deadline <= 0) { + return; + } + deadline -= Date.now(); + if (deadline <= 0) { + return; + } + + if (this.deadlineTimer) { + window.clearTimeout(this.deadlineTimer); + } + this.deadlineTimer = window.setTimeout(this.processDeadlines, deadline); + } + + private processDeadlines() { + this.deadlineTimer = 0; + try { + const now = Date.now(); + this.items.forEach(item => { + if (item.deadline && Math.abs(item.deadline - now) < 1000) { + this.retrying.delete(item); + item.resume(false); + } + }); + } + finally { + this.findDeadline(); + } + } + sorted(sids: number[]) { try { // Construct new items diff --git a/lib/manager/state.ts b/lib/manager/state.ts index 8b65d1f..dd815a5 100644 --- a/lib/manager/state.ts +++ b/lib/manager/state.ts @@ -8,8 +8,9 @@ export const PAUSED = 1 << 3; export const DONE = 1 << 4; export const CANCELED = 1 << 5; export const MISSING = 1 << 6; +export const RETRYING = 1 << 7; -export const RESUMABLE = PAUSED | CANCELED; -export const FORCABLE = PAUSED | QUEUED | CANCELED; -export const PAUSABLE = QUEUED | CANCELED | RUNNING; -export const CANCELABLE = QUEUED | RUNNING | PAUSED | DONE | MISSING; +export const RESUMABLE = PAUSED | CANCELED | RETRYING; +export const FORCABLE = PAUSED | QUEUED | CANCELED | RETRYING; +export const PAUSABLE = QUEUED | CANCELED | RUNNING | RETRYING; +export const CANCELABLE = QUEUED | RUNNING | PAUSED | DONE | MISSING | RETRYING; diff --git a/style/common.css b/style/common.css index e25e57f..3b032dd 100644 --- a/style/common.css +++ b/style/common.css @@ -10,6 +10,7 @@ --add-color: navy; --queue-color: gray; --pause-color: #ffa318; + --retry-color: rgb(0, 112, 204); --error-color: rgb(160, 13, 42); --running-color: #aae061; --finishing-color: #57cc12; diff --git a/style/manager.css b/style/manager.css index 06112ac..63d92fe 100644 --- a/style/manager.css +++ b/style/manager.css @@ -202,6 +202,23 @@ body > * { ); } +.retrying .virtualtable-column-2 .virtualtable-icon { + color: var(--retry-color); +} +.retrying .virtualtable-column-2 .virtualtable-progress-bar { + background: var(--retry-color); +} +.retrying .virtualtable-column-2 .virtualtable-progress-undetermined { + background: repeating-linear-gradient( + 45deg, + var(--retry-color), + var(--retry-color) 6px, + transparent 6px, + transparent 12px + ); +} + + .missing .virtualtable-column-2 .virtualtable-icon, .canceled .virtualtable-column-2 .virtualtable-icon { color: var(--error-color); diff --git a/style/prefs.css b/style/prefs.css index 702e86b..92e35fc 100644 --- a/style/prefs.css +++ b/style/prefs.css @@ -138,4 +138,11 @@ legend { border-radius: 6px; background: rgba(128, 128, 128, 0.05); box-shadow: 1px 1px 6px lightgray; +} + +#network-general { + display: grid; + grid-template-columns: auto 1fr; + grid-column-gap: 1em; + grid-row-gap: 1ex; } \ No newline at end of file diff --git a/windows/manager/state.ts b/windows/manager/state.ts index 5816d67..89c3076 100644 --- a/windows/manager/state.ts +++ b/windows/manager/state.ts @@ -10,6 +10,7 @@ export const StateTexts = locale.then(() => Object.freeze(new Map([ [DownloadState.QUEUED, _("queued")], [DownloadState.RUNNING, _("running")], [DownloadState.FINISHING, _("finishing")], + [DownloadState.RETRYING, _("paused")], [DownloadState.PAUSED, _("paused")], [DownloadState.DONE, _("done")], [DownloadState.CANCELED, _("canceled")], @@ -21,6 +22,7 @@ export const StateClasses = Object.freeze(new Map([ [DownloadState.RUNNING, "running"], [DownloadState.FINISHING, "finishing"], [DownloadState.PAUSED, "paused"], + [DownloadState.RETRYING, "retrying"], [DownloadState.DONE, "done"], [DownloadState.CANCELED, "canceled"], [DownloadState.MISSING, "missing"], @@ -31,6 +33,7 @@ export const StateIcons = Object.freeze(new Map([ [DownloadState.RUNNING, "icon-go"], [DownloadState.FINISHING, "icon-go"], [DownloadState.PAUSED, "icon-pause"], + [DownloadState.RETRYING, "icon-pause"], [DownloadState.DONE, "icon-done"], [DownloadState.CANCELED, "icon-error"], [DownloadState.MISSING, "icon-failed"], diff --git a/windows/manager/table.ts b/windows/manager/table.ts index 2ff62ca..649ec4a 100644 --- a/windows/manager/table.ts +++ b/windows/manager/table.ts @@ -146,6 +146,8 @@ export class DownloadItem extends EventEmitter { public opening: boolean; + public retries: number; + constructor(owner: DownloadTable, raw: any, stats?: Stats) { super(); Object.assign(this, raw); @@ -247,6 +249,12 @@ export class DownloadItem extends EventEmitter { if (this.state === DownloadState.RUNNING) { return this.eta; } + if (this.state === DownloadState.RETRYING) { + if (this.error) { + return _("retrying_error", _(this.error) || this.error); + } + return _("retrying"); + } if (this.error) { return _(this.error) || this.error; } diff --git a/windows/prefs.html b/windows/prefs.html index 5cddaa0..668e68f 100644 --- a/windows/prefs.html +++ b/windows/prefs.html @@ -123,9 +123,14 @@
-
+
- + + + + + +
diff --git a/windows/prefs.ts b/windows/prefs.ts index 7958c65..24e10ac 100644 --- a/windows/prefs.ts +++ b/windows/prefs.ts @@ -623,6 +623,8 @@ addEventListener("DOMContentLoaded", async () => { // Network new IntPref("pref-concurrent-downloads", "concurrent"); + new IntPref("pref-retries", "retries"); + new IntPref("pref-retry-time", "retry-time"); visible("#limits").then(() => new LimitsUI());