diff --git a/libraries/fetch-utils/index.ts b/libraries/fetch-utils/index.ts index ce1c06fdad..c54c217252 100644 --- a/libraries/fetch-utils/index.ts +++ b/libraries/fetch-utils/index.ts @@ -1,10 +1,12 @@ import _ from 'lodash' -import { Readable } from 'node:stream' +import { Readable, Duplex } from 'node:stream' import OError from '@overleaf/o-error' import fetch from 'node-fetch' import type { Response } from 'node-fetch' import http from 'node:http' import https from 'node:https' +import net from 'node:net' +import tls from 'node:tls' let logger: { warn: (...args: any[]) => void } | undefined @@ -160,6 +162,13 @@ class RequestFailedError extends OError { } } +function setupDefaultAgent(fetchOps: any) { + // Provide a function to get the agent for each request as there may be + // multiple requests with different protocols due to redirects. + fetchOps.agent = (url: URL) => + url.protocol === 'https:' ? httpsAgent : httpAgent +} + function parseOpts(opts: any, url: string | URL) { const fetchOpts = _.omit(opts, ['json', 'signal', 'basicAuth']) if (opts.json) { @@ -168,6 +177,9 @@ function parseOpts(opts: any, url: string | URL) { if (opts.basicAuth) { setupBasicAuth(fetchOpts, opts.basicAuth) } + if (!fetchOpts.agent) { + setupDefaultAgent(fetchOpts) + } const abortController = new AbortController() fetchOpts.signal = abortController.signal @@ -302,34 +314,89 @@ class ConnectTimeoutError extends OError { } } -function withTimeout(createConnection: any, options: any, callback: any) { - if (options.connectTimeout) { - // Wrap createConnection in a timeout - const timer = setTimeout(() => { - socket.destroy(new ConnectTimeoutError(options)) - }, options.connectTimeout) - const socket = createConnection(options, (err: any, stream: any) => { - clearTimeout(timer) - callback(err, stream) - }) - return socket - } else { - // Fallback to default createConnection - return createConnection(options, callback) +function tryToCreateConnection( + createConnection: (options: any) => Duplex, + options: any, + callback: (err: any, socket: any) => void +) { + let socket: Duplex + try { + socket = createConnection(options) + } catch (err) { + callback(err, null) + return } + const timer = setTimeout(() => { + socket.destroy(new ConnectTimeoutError(options)) + }, options.connectTimeout) + const onConnect = () => { + clearTimeout(timer) + socket.off('error', onError) + callback(null, socket) + } + const onError = (err: any) => { + clearTimeout(timer) + socket.off('connect', onConnect) + callback(err, null) + } + socket.once('connect', onConnect) + socket.once('error', onError) +} + +function withTimeout( + createConnection: (options: any) => Duplex, + options: any, + callback: (err: any, socket: any) => void +) { + const attempt = (remainingAttempts: number) => { + remainingAttempts-- + tryToCreateConnection(createConnection, options, (err, socket) => { + if (err && remainingAttempts > 0) { + setTimeout(() => { + attempt(remainingAttempts) + }, options.connectRetryInterval ?? 100) + return + } + callback(err, socket) + }) + } + attempt(3) } class CustomHttpAgent extends http.Agent { + constructor(options: any) { + if (!(options.connectTimeout > 0)) { + throw new Error( + 'CustomHttpAgent must be called with positive connectTimeout' + ) + } + super(options) + } createConnection(options: any, callback: any) { - return withTimeout(super.createConnection.bind(this), options, callback) + withTimeout(net.createConnection, options, callback) + return undefined } } + class CustomHttpsAgent extends https.Agent { + constructor(options: any) { + if (!(options.connectTimeout > 0)) { + throw new Error( + 'CustomHttpsAgent must be called with positive connectTimeout' + ) + } + super(options) + } createConnection(options: any, callback: any) { - return withTimeout(super.createConnection.bind(this), options, callback) + withTimeout(tls.connect, options, callback) + return undefined } } +const MAX_CONNECT_TIME = 1000 +const httpAgent = new CustomHttpAgent({ connectTimeout: MAX_CONNECT_TIME }) +const httpsAgent = new CustomHttpsAgent({ connectTimeout: MAX_CONNECT_TIME }) + export { fetchJson, fetchJsonWithResponse, diff --git a/libraries/fetch-utils/test/unit/FetchUtils.test.js b/libraries/fetch-utils/test/unit/FetchUtils.test.js index 4f0a5fcd59..37c101bf99 100644 --- a/libraries/fetch-utils/test/unit/FetchUtils.test.js +++ b/libraries/fetch-utils/test/unit/FetchUtils.test.js @@ -1,5 +1,6 @@ import { expect } from 'chai' import dns from 'node:dns' +import http from 'node:http' import { once } from 'node:events' import events from 'node:events' import fs from 'node:fs' @@ -364,6 +365,66 @@ describe('fetch-utils', function () { 'request to http://10.255.255.255/ failed, reason: connect timeout' ) }) + + it('retries after a delay', { timeout: 5_000 }, async function () { + // The test should finish after 1.5s. + // t 0ms: first connect fails with ECONNREFUSED + // t 500ms: listen on port + // t1000ms: retry to connect again, works + // t1500ms: respond to request + const agent = new CustomHttpAgent({ + connectTimeout: 100, + connectRetryInterval: 1_000, + }) + const server = http.createServer((req, res) => { + setTimeout(() => { + res.write('hello') + res.end() + server.close() + }, 500) + }) + // Grab a dynamic port, then close again so that the first connect + // attempt is refused before the server starts listening for real. + await once(server.listen(0), 'listening') + const { port } = server.address() + await new Promise(resolve => server.close(resolve)) + const t0 = performance.now() + setTimeout(() => { + server.listen(port) + }, 500) + const body = await fetchString(`http://127.0.0.1:${port}`, { agent }) + expect(body).to.equal('hello') + const t1 = performance.now() + expect(t1 - t0).to.be.at.least(1_500) + }) + + it('does not open a stray connection when the socket errors after connect', async function () { + const agent = new CustomHttpAgent({ + connectTimeout: 100, + connectRetryInterval: 10, + }) + const connections = [] + const server = http.createServer(req => { + // Reset the established connection before responding. Doing this in + // the request handler guarantees the client finished the connect + // phase, so the ECONNRESET arrives after 'connect' has fired. + req.socket.resetAndDestroy() + }) + server.on('connection', socket => connections.push(socket)) + await once(server.listen(0), 'listening') + const { port } = server.address() + try { + await expect( + fetchString(`http://127.0.0.1:${port}/`, { agent }) + ).to.be.rejectedWith(FetchError) + // Leave time for a buggy connect retry to open a stray connection. + await wait(200) + expect(connections).to.have.length(1) + } finally { + for (const socket of connections) socket.destroy() + server.close() + } + }) }) describe('CustomHttpsAgent', function () { diff --git a/services/clsi/app/js/UrlFetcher.js b/services/clsi/app/js/UrlFetcher.js index 70faed00dd..aff59df9f5 100644 --- a/services/clsi/app/js/UrlFetcher.js +++ b/services/clsi/app/js/UrlFetcher.js @@ -1,20 +1,11 @@ import fs from 'node:fs' import logger from '@overleaf/logger' import Settings from '@overleaf/settings' -import { - CustomHttpAgent, - CustomHttpsAgent, - fetchStream, - RequestFailedError, -} from '@overleaf/fetch-utils' +import { fetchStream, RequestFailedError } from '@overleaf/fetch-utils' import { URL } from 'node:url' import { pipeline } from 'node:stream/promises' import Metrics from '@overleaf/metrics' -const MAX_CONNECT_TIME = 1000 -const httpAgent = new CustomHttpAgent({ connectTimeout: MAX_CONNECT_TIME }) -const httpsAgent = new CustomHttpsAgent({ connectTimeout: MAX_CONNECT_TIME }) - async function pipeUrlToFileWithRetry(url, fallbackURL, filePath) { let remainingAttempts = 3 let lastErr @@ -60,10 +51,6 @@ async function pipeUrlToFile(url, fallbackURL, filePath) { try { stream = await fetchStream(url, { signal: AbortSignal.timeout(60 * 1000), - // provide a function to get the agent for each request - // as there may be multiple requests with different protocols - // due to redirects. - agent: _url => (_url.protocol === 'https:' ? httpsAgent : httpAgent), }) } catch (err) { if ( @@ -73,10 +60,6 @@ async function pipeUrlToFile(url, fallbackURL, filePath) { ) { stream = await fetchStream(fallbackURL, { signal: AbortSignal.timeout(60 * 1000), - // provide a function to get the agent for each request - // as there may be multiple requests with different protocols - // due to redirects. - agent: _url => (_url.protocol === 'https:' ? httpsAgent : httpAgent), }) url = fallbackURL } else {