From 58c69e63af126e032430ffeb757c52fa8054d3bd Mon Sep 17 00:00:00 2001 From: Stefan Guggisberg Date: Wed, 10 Apr 2024 15:47:24 +0200 Subject: [PATCH] fix: disable h1 keep-alive by default (#469) * fix: disable h1 keep-alive by default * fix: tweak tests for changed behavior of node >= v20 --- src/core/request.js | 4 +++- src/fetch/index.js | 25 +++++++++++++++++-------- src/index.d.ts | 10 ++++++---- test/fetch/index.http1.test.js | 26 ++++++++++++-------------- test/fetch/resiliance.test.js | 4 ++-- 5 files changed, 40 insertions(+), 29 deletions(-) diff --git a/src/core/request.js b/src/core/request.js index a8b7f56..ef03dff 100644 --- a/src/core/request.js +++ b/src/core/request.js @@ -275,7 +275,9 @@ const request = async (ctx, uri, options) => { return await h2.request(ctx, url, socket ? { ...opts, socket } : opts); } catch (err) { const { code, message } = err; - if (code === 'ERR_HTTP2_ERROR' && message === 'Protocol error') { + /* c8 ignore next 2 */ + if ((code === 'ERR_HTTP2_ERROR' && message === 'Protocol error') + || code === 'ERR_HTTP2_STREAM_CANCEL') { // server potentially downgraded from h2 to h1: clear alpn cache entry ctx.alpnCache.delete(`${url.protocol}//${url.host}`); } diff --git a/src/fetch/index.js b/src/fetch/index.js index 614071a..e5878b3 100644 --- a/src/fetch/index.js +++ b/src/fetch/index.js @@ -442,20 +442,23 @@ class FetchContext { noCache: (options = {}) => new FetchContext({ ...options, maxCacheSize: 0 }).api(), /** - * Convenience function which creates a new context with enforced HTTP/1.1 protocol, - * the equivalent of `context({ alpnProtocols: [ALPN_HTTP1_1] })`. + * Convenience function which creates a new context with enforced HTTP/1.1 protocol + * and disabled persistent connections (keep-alive), the equivalent of + * `context({ alpnProtocols: [ALPN_HTTP1_1], h1: { keepAlive: false } })`. * * The optional `options` parameter allows to specify further options. * * @param {Object} [options={}] */ h1: (options = {}) => new FetchContext({ - ...options, alpnProtocols: [this.context.ALPN_HTTP1_1], + ...options, + alpnProtocols: [this.context.ALPN_HTTP1_1], + h1: { keepAlive: false }, }).api(), /** * Convenience function which creates a new context with enforced HTTP/1.1 protocol - * and persistent connections (keep-alive), the equivalent of + * with persistent connections (keep-alive), the equivalent of * `context({ alpnProtocols: [ALPN_HTTP1_1], h1: { keepAlive: true } })`. * * The optional `options` parameter allows to specify further options. @@ -463,19 +466,25 @@ class FetchContext { * @param {Object} [options={}] */ keepAlive: (options = {}) => new FetchContext({ - ...options, alpnProtocols: [this.context.ALPN_HTTP1_1], h1: { keepAlive: true }, + ...options, + alpnProtocols: [this.context.ALPN_HTTP1_1], + h1: { keepAlive: true }, }).api(), /** - * Convenience function which creates a new context with disabled caching - * and enforced HTTP/1.1 protocol, a combination of `h1()` and `noCache()`. + * Convenience function which creates a new context with disabled caching, + * enforced HTTP/1.1 protocol and disabled persistent connections (keep-alive), + * a combination of `h1()` and `noCache()`. * * The optional `options` parameter allows to specify further options. * * @param {Object} [options={}] */ h1NoCache: (options = {}) => new FetchContext({ - ...options, maxCacheSize: 0, alpnProtocols: [this.context.ALPN_HTTP1_1], + ...options, + maxCacheSize: 0, + alpnProtocols: [this.context.ALPN_HTTP1_1], + h1: { keepAlive: false }, }).api(), /** diff --git a/src/index.d.ts b/src/index.d.ts index 68007ab..11e83d8 100644 --- a/src/index.d.ts +++ b/src/index.d.ts @@ -36,8 +36,9 @@ export declare function context(options?: ContextOptions): FetchAPI; export declare function noCache(options?: ContextOptions): FetchAPI; /** - * Convenience function which creates a new context with enforced HTTP/1.1 protocol, - * the equivalent of `context({ alpnProtocols: [ALPN_HTTP1_1] })`. + * Convenience function which creates a new context with enforced HTTP/1.1 protocol + * and disabled persistent connections (keep-alive), the equivalent of + * `context({ alpnProtocols: [ALPN_HTTP1_1], h1: { keepAlive: false } })`. * * The optional `options` parameter allows to specify further options. * @@ -57,8 +58,9 @@ export declare function noCache(options?: ContextOptions): FetchAPI; export declare function keepAlive(options?: ContextOptions): FetchAPI; /** - * Convenience function which creates a new context with disabled caching - * and enforced HTTP/1.1 protocol, a combination of `h1()` and `noCache()`. + * Convenience function which creates a new context with disabled caching, + * and enforced HTTP/1.1 protocol with disabled persistent connections (keep-alive), + * a combination of `h1()` and `noCache()`. * * The optional `options` parameter allows to specify further options. * diff --git a/test/fetch/index.http1.test.js b/test/fetch/index.http1.test.js index d3136bf..a21b4b6 100644 --- a/test/fetch/index.http1.test.js +++ b/test/fetch/index.http1.test.js @@ -79,6 +79,18 @@ testParams.forEach((params) => { } }); + it(`h1() defaults to 'no keep-alive' (${name})`, async () => { + const { fetch, reset } = h1({ rejectUnauthorized: false }); + try { + const resp = await fetch(`${server.origin}/status/200`); + assert.strictEqual(resp.status, 200); + assert.strictEqual(resp.httpVersion, '1.1'); + assert.strictEqual(resp.headers.get('connection'), 'close'); + } finally { + await reset(); + } + }); + it(`supports h1NoCache() (${name})`, async () => { const { fetch, cacheStats, reset } = h1NoCache({ rejectUnauthorized: false }); try { @@ -99,20 +111,6 @@ testParams.forEach((params) => { } }); - it(`defaults to 'no keep-alive' (${name})`, async () => { - const { fetch, reset } = context( - { alpnProtocols: [ALPN_HTTP1_1], rejectUnauthorized: false }, - ); - try { - const resp = await fetch(`${server.origin}/status/200`); - assert.strictEqual(resp.status, 200); - assert.strictEqual(resp.httpVersion, '1.1'); - assert.strictEqual(resp.headers.get('connection'), 'close'); - } finally { - await reset(); - } - }); - it(`supports h1.keepAlive context option (${name})`, async () => { const { fetch, reset } = context( { alpnProtocols: [ALPN_HTTP1_1], h1: { keepAlive: true }, rejectUnauthorized: false }, diff --git a/test/fetch/resiliance.test.js b/test/fetch/resiliance.test.js index 8c213e5..a78616c 100644 --- a/test/fetch/resiliance.test.js +++ b/test/fetch/resiliance.test.js @@ -63,8 +63,8 @@ describe('Fetch Resiliance Tests', () => { process.kill(server.pid); // start h1 server server = await Server.launch(1, true, HELLO_MSG, server.port); - // expect FetchError: Protocol error - await assert.rejects(ctx.fetch(`${server.origin}/hello`), { name: 'FetchError', message: 'Protocol error' }); + // expect FetchError: Protocol error (message depends on node version) + await assert.rejects(ctx.fetch(`${server.origin}/hello`), { name: 'FetchError' }); // the fetch context should have recovered by now, next request should succeed resp = await ctx.fetch(`${server.origin}/hello`); assert.strictEqual(resp.status, 200);