From fe9cdfbed90a7bc29d2304afb2811e3072e8a13a Mon Sep 17 00:00:00 2001 From: Fedor Indutny <79877362+indutny-signal@users.noreply.github.com> Date: Wed, 2 Mar 2022 11:48:07 -0800 Subject: [PATCH] Fix error handling during piping in updater --- ts/test-node/updater/differential_test.ts | 50 ++++++++++++++++++++--- ts/updater/common.ts | 12 +++--- ts/updater/differential.ts | 29 ++++++++++--- ts/updater/got.ts | 7 ++-- 4 files changed, 77 insertions(+), 21 deletions(-) diff --git a/ts/test-node/updater/differential_test.ts b/ts/test-node/updater/differential_test.ts index 0b3a868c7b..1781274f4d 100644 --- a/ts/test-node/updater/differential_test.ts +++ b/ts/test-node/updater/differential_test.ts @@ -8,6 +8,8 @@ import fs from 'fs/promises'; import { tmpdir } from 'os'; import { strictAssert } from '../../util/assert'; +import * as durations from '../../util/durations'; +import { getGotOptions } from '../../updater/got'; import { computeDiff, getBlockMapFileName, @@ -73,8 +75,10 @@ describe('updater/differential', () => { let server: http.Server; let baseUrl: string; + let shouldTimeout: 'response' | undefined; beforeEach(callback => { + shouldTimeout = undefined; server = http.createServer(async (req, res) => { if (!req.headers['user-agent']?.includes('Signal-Desktop')) { res.writeHead(403); @@ -107,11 +111,13 @@ describe('updater/differential', () => { const BOUNDARY = 'f8f254ce1ba37627'; - res.setHeader( - 'content-type', - `multipart/byteranges; boundary=${BOUNDARY}` - ); - res.writeHead(206); + res.writeHead(206, { + 'content-type': `multipart/byteranges; boundary=${BOUNDARY}`, + }); + if (shouldTimeout === 'response') { + res.flushHeaders(); + return; + } const totalSize = fullFile.length; @@ -256,7 +262,11 @@ describe('updater/differential', () => { const outFile = path.join(outDir, 'out.bin'); const chunks = new Array(); - await download(outFile, data, size => chunks.push(size)); + await download(outFile, data, { + statusCallback(size) { + chunks.push(size); + }, + }); const expected = await fs.readFile(path.join(FIXTURES, newFile)); const actual = await fs.readFile(outFile); @@ -267,5 +277,33 @@ describe('updater/differential', () => { 'Expected multiple callback invocations' ); }); + + it('handles response timeouts gracefully', async () => { + const data = await prepareDownload({ + oldFile: path.join(FIXTURES, oldFile), + newUrl: `${baseUrl}/${newFile}`, + sha512: newHash, + }); + + const outDir = await fs.mkdtemp(path.join(tmpdir(), 'signal-temp-')); + await fs.mkdir(outDir, { recursive: true }); + + const outFile = path.join(outDir, 'out.bin'); + + shouldTimeout = 'response'; + await assert.isRejected( + download(outFile, data, { + gotOptions: { + ...getGotOptions(), + timeout: { + connect: 0.5 * durations.SECOND, + lookup: 0.5 * durations.SECOND, + socket: 0.5 * durations.SECOND, + }, + }, + }), + /Timeout awaiting 'socket' for 500ms/ + ); + }); }); }); diff --git a/ts/updater/common.ts b/ts/updater/common.ts index 955a421b40..595043aa89 100644 --- a/ts/updater/common.ts +++ b/ts/updater/common.ts @@ -484,12 +484,12 @@ export abstract class Updater { ); try { - await downloadDifferentialData( - targetUpdatePath, - differentialData, - updateOnProgress ? this.throttledSendDownloadingUpdate : undefined, - this.logger - ); + await downloadDifferentialData(targetUpdatePath, differentialData, { + statusCallback: updateOnProgress + ? this.throttledSendDownloadingUpdate + : undefined, + logger: this.logger, + }); gotUpdate = true; } catch (error) { diff --git a/ts/updater/differential.ts b/ts/updater/differential.ts index f89739e18e..7f9b9b87ea 100644 --- a/ts/updater/differential.ts +++ b/ts/updater/differential.ts @@ -70,6 +70,14 @@ export type PrepareDownloadOptionsType = Readonly<{ sha512: string; }>; +export type DownloadOptionsType = Readonly<{ + statusCallback?: (downloadedSize: number) => void; + logger?: LoggerType; + + // Testing + gotOptions?: ReturnType; +}>; + export type DownloadRangesOptionsType = Readonly<{ url: string; output: FileHandle; @@ -77,6 +85,9 @@ export type DownloadRangesOptionsType = Readonly<{ logger?: LoggerType; abortSignal?: AbortSignal; chunkStatusCallback: (chunkSize: number) => void; + + // Testing + gotOptions?: ReturnType; }>; export function getBlockMapFileName(fileName: string): string { @@ -240,8 +251,7 @@ export function isValidPreparedData( export async function download( newFile: string, { diff, oldFile, newUrl, sha512 }: PrepareDownloadResultType, - statusCallback?: (downloadedSize: number) => void, - logger?: LoggerType + { statusCallback, logger, gotOptions }: DownloadOptionsType = {} ): Promise { const input = await open(oldFile, 'r'); @@ -288,6 +298,7 @@ export async function download( ranges: downloadActions, logger, abortSignal, + gotOptions, chunkStatusCallback(chunkSize) { downloadedSize += chunkSize; if (!abortSignal.aborted) { @@ -336,7 +347,14 @@ export async function downloadRanges( } // Request multiple ranges in a single request - const { url, output, logger, abortSignal, chunkStatusCallback } = options; + const { + url, + output, + logger, + abortSignal, + chunkStatusCallback, + gotOptions = getGotOptions(), + } = options; logger?.info('updater/downloadRanges: downloading ranges', ranges.length); @@ -350,8 +368,7 @@ export async function downloadRanges( diffByRange.set(`${readOffset}-${readOffset + size - 1}`, diff); } - const gotOptions = getGotOptions(); - const stream = got.stream(`${url}`, { + const stream = got.stream(url, { ...gotOptions, headers: { ...gotOptions.headers, @@ -402,6 +419,7 @@ export async function downloadRanges( dicer.on('part', part => partPromises.push(onPart(part))); dicer.once('finish', () => stream.destroy()); + stream.once('error', err => dicer.destroy(err)); // Pipe the response stream fully into dicer // NOTE: we can't use `pipeline` due to a dicer bug: @@ -425,7 +443,6 @@ export async function downloadRanges( return; } - throw new Error('Missing ranges'); logger?.info( 'updater/downloadRanges: downloading missing ranges', diffByRange.size diff --git a/ts/updater/got.ts b/ts/updater/got.ts index 9d4e785790..c85e83121e 100644 --- a/ts/updater/got.ts +++ b/ts/updater/got.ts @@ -7,10 +7,11 @@ import ProxyAgent from 'proxy-agent'; import * as packageJson from '../../package.json'; import { getUserAgent } from '../util/getUserAgent'; +import * as durations from '../util/durations'; -export const GOT_CONNECT_TIMEOUT = 2 * 60 * 1000; -export const GOT_LOOKUP_TIMEOUT = 2 * 60 * 1000; -export const GOT_SOCKET_TIMEOUT = 2 * 60 * 1000; +export const GOT_CONNECT_TIMEOUT = 5 * durations.MINUTE; +export const GOT_LOOKUP_TIMEOUT = 5 * durations.MINUTE; +export const GOT_SOCKET_TIMEOUT = 5 * durations.MINUTE; export function getProxyUrl(): string | undefined { return process.env.HTTPS_PROXY || process.env.https_proxy;