-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat: Provide progress updates for multiple range differential downloads #9448
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "electron-updater": minor | ||
| --- | ||
|
|
||
| Emit download-progress events when downloading with multiple range differential downloader |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ import { newError } from "builder-util-runtime" | |
| import { createReadStream } from "fs" | ||
| import { Writable } from "stream" | ||
| import { Operation, OperationKind } from "./downloadPlanBuilder" | ||
| import { ProgressInfo } from "./ProgressDifferentialDownloadCallbackTransform" | ||
|
|
||
| const DOUBLE_CRLF = Buffer.from("\r\n\r\n") | ||
|
|
||
|
|
@@ -34,6 +35,11 @@ export function copyData(task: Operation, out: Writable, oldFileFd: number, reje | |
| } | ||
|
|
||
| export class DataSplitter extends Writable { | ||
| private start = Date.now() | ||
| private nextUpdate = this.start + 1000 | ||
| private transferred = 0 | ||
| private delta = 0 | ||
|
|
||
| partIndex = -1 | ||
|
|
||
| private headerListBuffer: Buffer | null = null | ||
|
|
@@ -49,7 +55,9 @@ export class DataSplitter extends Writable { | |
| private readonly partIndexToTaskIndex: Map<number, number>, | ||
| boundary: string, | ||
| private readonly partIndexToLength: Array<number>, | ||
| private readonly finishHandler: () => any | ||
| private readonly finishHandler: () => any, | ||
| private readonly grandTotalBytes: number, | ||
| private readonly onProgress?: (info: ProgressInfo) => any | ||
| ) { | ||
| super() | ||
|
|
||
|
|
@@ -69,7 +77,27 @@ export class DataSplitter extends Writable { | |
| return | ||
| } | ||
|
|
||
| this.handleData(data).then(callback).catch(callback) | ||
| this.handleData(data) | ||
| .then(() => { | ||
| if (this.onProgress) { | ||
| const now = Date.now() | ||
| if (now >= this.nextUpdate || this.transferred === this.grandTotalBytes) { | ||
| this.nextUpdate = now + 1000 | ||
|
|
||
| this.onProgress({ | ||
| total: this.grandTotalBytes, | ||
| delta: this.delta, | ||
| transferred: this.transferred, | ||
| percent: (this.transferred / this.grandTotalBytes) * 100, | ||
| bytesPerSecond: Math.round(this.transferred / ((now - this.start) / 1000)), | ||
|
||
| }) | ||
| this.delta = 0 | ||
| } | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the simplicity of this PR in that it's throttling/firing a callback when data is received (as opposed to a NodeJS.Timer approach), but I'm concerned with the number of properties that are being added. Feels like we're recreating a wheel here but I can't think of a better approach. Can you at least please add a short tsdoc as to what each is doing (or expected to be used as/for)? |
||
|
|
||
| callback() | ||
| }) | ||
| .catch(callback) | ||
| } | ||
|
|
||
| private async handleData(chunk: Buffer): Promise<undefined> { | ||
|
|
@@ -217,6 +245,8 @@ export class DataSplitter extends Writable { | |
|
|
||
| private processPartData(data: Buffer, start: number, end: number): Promise<void> { | ||
| this.actualPartLength += end - start | ||
| this.transferred += end - start | ||
| this.delta += end - start | ||
| const out = this.out | ||
| if (out.write(start === 0 && data.length === end ? data : data.slice(start, end))) { | ||
| return Promise.resolve() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Division by zero can occur if grandTotalBytes is 0. While this scenario might be unlikely, if all tasks are COPY operations or if there are no download tasks, grandTotalBytes could be 0, causing the percent calculation to result in NaN or Infinity.