fix: disable h1 keep-alive by default (#469)

* fix: disable h1 keep-alive by default * fix: tweak tests for changed behavior of node >= v20
adobe · Apr 10, 2024 · 58c69e6 · 58c69e6
1 parent 6fc1baf
commit 58c69e6
Show file tree

Hide file tree

Showing 5 changed files with 40 additions and 29 deletions.
diff --git 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
@@ -442,40 +442,49 @@ 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.
        *
        * @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
@@ -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
@@ -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
@@ -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);