fix: should HEAD request keepalive by default

closes #565
node-modules · Dec 19, 2024 · 68f5713 · 68f5713
1 parent 26764a3
commit 68f5713
Show file tree

Hide file tree

Showing 8 changed files with 223 additions and 16 deletions.
diff --git a/src/HttpClient.ts b/src/HttpClient.ts
@@ -428,6 +428,7 @@ export class HttpClient extends EventEmitter {
         opaque: internalOpaque,
         dispatcher: args.dispatcher ?? this.#dispatcher,
         signal: args.signal,
+        reset: false,
       };
       if (typeof args.highWaterMark === 'number') {
         requestOptions.highWaterMark = args.highWaterMark;

diff --git a/src/Request.ts b/src/Request.ts
@@ -148,7 +148,7 @@ export type RequestOptions = {
    * unix domain socket file path
    */
   socketPath?: string | null;
-  /** Whether the request should stablish a keep-alive or not. Default `undefined` */
+  /** Whether the request should stablish a keep-alive or not. Default `false`, try to keep alive by default */
   reset?: boolean;
   /** Default: `64 KiB` */
   highWaterMark?: number;

diff --git a/src/diagnosticsChannel.ts b/src/diagnosticsChannel.ts
@@ -3,10 +3,12 @@ import { performance } from 'node:perf_hooks';
 import { debuglog } from 'node:util';
 import { Socket } from 'node:net';
 import { DiagnosticsChannel } from 'undici';
+// eslint-disable-next-line @typescript-eslint/ban-ts-comment
+// @ts-ignore
 import symbols from './symbols.js';
 import { globalId, performanceTime } from './utils.js';
 
-const debug = debuglog('urllib:DiagnosticsChannel');
+const debug = debuglog('urllib/diagnosticsChannel');
 let initedDiagnosticsChannel = false;
 // https://undici.nodejs.org/#/docs/api/DiagnosticsChannel
 // client --> server
@@ -24,18 +26,29 @@ function subscribe(name: string, listener: (message: unknown, channelName: strin
 }
 
 type SocketExtend = Socket & {
-  [key: symbol]: string | number | Date | undefined;
+  [key: symbol]: string | number | Date | undefined | boolean;
 };
 
+let kSocketReset: symbol;
 function formatSocket(socket: SocketExtend) {
   if (!socket) return socket;
+  if (!kSocketReset) {
+    const symbols = Object.getOwnPropertySymbols(socket);
+    for (const symbol of symbols) {
+      if (symbol.description === 'reset') {
+        kSocketReset = symbol;
+        break;
+      }
+    }
+  }
   return {
     localAddress: socket[symbols.kSocketLocalAddress],
     localPort: socket[symbols.kSocketLocalPort],
     remoteAddress: socket.remoteAddress,
     remotePort: socket.remotePort,
     attemptedAddresses: socket.autoSelectFamilyAttemptedAddresses,
     connecting: socket.connecting,
+    reset: socket[kSocketReset],
   };
 }
 

diff --git a/src/fetch.ts b/src/fetch.ts
@@ -42,7 +42,7 @@ import { RawResponseWithMeta, SocketInfo } from './Response.js';
 import { IncomingHttpHeaders } from './IncomingHttpHeaders.js';
 import { BaseAgent, BaseAgentOptions } from './BaseAgent.js';
 
-const debug = debuglog('urllib:fetch');
+const debug = debuglog('urllib/fetch');
 
 export interface UrllibRequestInit extends RequestInit {
   // default is true

diff --git a/test/diagnostics_channel.test.ts b/test/diagnostics_channel.test.ts
@@ -47,6 +47,7 @@ describe('diagnostics_channel.test.ts', () => {
         }
       }
       const handler = request[kHandler];
+      if (!handler) return;
       let opaque = handler.opaque || handler.opts?.opaque;
       assert(opaque);
       opaque = opaque[symbols.kRequestOriginalOpaque];
@@ -88,7 +89,7 @@ describe('diagnostics_channel.test.ts', () => {
     assert(lastRequestOpaque.tracer.socket);
     assert.equal(lastRequestOpaque.tracer.socket.requests, 1);
 
-    // HEAD 请求不会 keepalive
+    // HEAD 请求会走 keepalive
     // GET 请求会走 keepalive
     await sleep(1);
     traceId = `mock-traceid-${Date.now()}`;
@@ -102,7 +103,7 @@ describe('diagnostics_channel.test.ts', () => {
     assert.equal(response.status, 200);
     assert.equal(lastRequestOpaque.tracer.traceId, traceId);
     assert(lastRequestOpaque.tracer.socket);
-    assert.equal(lastRequestOpaque.tracer.socket.requests, 1);
+    assert.equal(lastRequestOpaque.tracer.socket.requests, 2);
 
     await sleep(1);
     traceId = `mock-traceid-${Date.now()}`;
@@ -116,7 +117,7 @@ describe('diagnostics_channel.test.ts', () => {
     assert.equal(response.status, 200);
     assert.equal(lastRequestOpaque.tracer.traceId, traceId);
     assert(lastRequestOpaque.tracer.socket);
-    assert.equal(lastRequestOpaque.tracer.socket.requests, 2);
+    assert.equal(lastRequestOpaque.tracer.socket.requests, 3);
 
     // socket 复用 1000 次
     let count = 1000;
@@ -133,7 +134,7 @@ describe('diagnostics_channel.test.ts', () => {
       assert.equal(response.status, 200);
       assert.equal(lastRequestOpaque.tracer.traceId, traceId);
       assert(lastRequestOpaque.tracer.socket);
-      assert.equal(lastRequestOpaque.tracer.socket.requests, 2 + 1000 - count);
+      assert.equal(lastRequestOpaque.tracer.socket.requests, 3 + 1000 - count);
     }
 
     diagnosticsChannel.unsubscribe('undici:client:connected', onMessage);
@@ -311,7 +312,7 @@ describe('diagnostics_channel.test.ts', () => {
     assert.equal(socket.handledResponses, 1);
     assert.equal(lastRequestOpaque.tracer.traceId, traceId);
 
-    // HEAD 请求不会 keepalive
+    // HEAD 请求会走 keepalive
     // GET 请求会走 keepalive
     await sleep(1);
     traceId = `mock-traceid-${Date.now()}`;
@@ -325,8 +326,8 @@ describe('diagnostics_channel.test.ts', () => {
     assert.equal(response.status, 200);
     assert.equal(lastRequestOpaque.tracer.traceId, traceId);
     assert(socket);
-    assert.equal(socket.handledRequests, 1);
-    assert.equal(socket.handledResponses, 1);
+    assert.equal(socket.handledRequests, 2);
+    assert.equal(socket.handledResponses, 2);
 
     await sleep(1);
     traceId = `mock-traceid-${Date.now()}`;
@@ -340,8 +341,8 @@ describe('diagnostics_channel.test.ts', () => {
     assert.equal(response.status, 200);
     assert.equal(lastRequestOpaque.tracer.traceId, traceId);
     assert(socket);
-    assert.equal(socket.handledRequests, 2);
-    assert.equal(socket.handledResponses, 2);
+    assert.equal(socket.handledRequests, 3);
+    assert.equal(socket.handledResponses, 3);
 
     // socket 复用 1000 次
     let count = 1000;
@@ -358,8 +359,8 @@ describe('diagnostics_channel.test.ts', () => {
       assert.equal(response.status, 200);
       assert.equal(lastRequestOpaque.tracer.traceId, traceId);
       assert(socket);
-      assert.equal(socket.handledRequests, 2 + 1000 - count);
-      assert.equal(socket.handledResponses, 2 + 1000 - count);
+      assert.equal(socket.handledRequests, 3 + 1000 - count);
+      assert.equal(socket.handledResponses, 3 + 1000 - count);
     }
 
     diagnosticsChannel.unsubscribe('urllib:request', onRequestMessage);

diff --git a/test/fetch-head-request-should-keepalive.test.ts b/test/fetch-head-request-should-keepalive.test.ts
@@ -0,0 +1,94 @@
+import { strict as assert } from 'node:assert';
+import { scheduler } from 'node:timers/promises';
+import { describe, it, beforeAll, afterAll } from 'vitest';
+import { fetch } from '../src/index.js';
+import { startServer } from './fixtures/server.js';
+
+describe.skip('fetch-head-request-should-keepalive.test.ts', () => {
+  // https://github.com/node-modules/urllib/issues/565
+  // head request should keepalive
+  let close: any;
+  let _url: string;
+  beforeAll(async () => {
+    const { closeServer, urlWithDns } = await startServer();
+    close = closeServer;
+    _url = urlWithDns;
+  });
+
+  afterAll(async () => {
+    await close();
+  });
+
+  it('should keepalive on GET => HEAD => HEAD Request', async () => {
+    let res = await fetch(_url, {
+      method: 'GET',
+    });
+    assert.equal(res.status, 200);
+    console.log(res.headers, res);
+    assert.equal(res.headers.get('connection'), 'keep-alive');
+
+    await scheduler.wait(10);
+    res = await fetch(_url, {
+      method: 'HEAD',
+    });
+    assert.equal(res.status, 200);
+    console.log(res.headers, res);
+    assert.equal(res.headers.get('connection'), 'keep-alive');
+
+    // await scheduler.wait(1);
+    // res = await httpClient.request(_url, {
+    //   method: 'HEAD',
+    // });
+    // assert.equal(res.status, 200);
+    // // console.log(res.headers, res.res.socket);
+    // assert.equal(res.headers.connection, 'keep-alive');
+    // assert.equal(res.res.socket.id, socketId);
+
+    // res = await httpClient.request(_url, {
+    //   method: 'HEAD',
+    // });
+    // assert.equal(res.status, 200);
+    // // console.log(res.headers, res.res.socket);
+    // assert.equal(res.headers.connection, 'keep-alive');
+
+    // await scheduler.wait(1);
+    // res = await httpClient.request(_url, {
+    //   method: 'HEAD',
+    // });
+    // assert.equal(res.status, 200);
+    // // console.log(res.headers, res.res.socket);
+    // assert.equal(res.headers.connection, 'keep-alive');
+    // assert.equal(res.res.socket.id, socketId);
+
+    // await scheduler.wait(1);
+    // res = await httpClient.request(_url, {
+    //   method: 'HEAD',
+    // });
+    // assert.equal(res.status, 200);
+    // // console.log(res.headers, res.res.socket);
+    // assert.equal(res.headers.connection, 'keep-alive');
+    // assert.equal(res.res.socket.id, socketId);
+  });
+
+  // it('should close connection when reset = true', async () => {
+  //   const httpClient = new HttpClient();
+  //   let res = await httpClient.request(_url, {
+  //     method: 'GET',
+  //     reset: true,
+  //   });
+  //   assert.equal(res.status, 200);
+  //   // console.log(res.headers, res.res.socket);
+  //   assert.equal(res.headers.connection, 'close');
+  //   const socketId = res.res.socket.id;
+
+  //   await scheduler.wait(10);
+  //   res = await httpClient.request(_url, {
+  //     method: 'HEAD',
+  //     reset: true,
+  //   });
+  //   assert.equal(res.status, 200);
+  //   // console.log(res.headers, res.res.socket);
+  //   assert.equal(res.headers.connection, 'close');
+  //   assert.notEqual(res.res.socket.id, socketId);
+  // });
+});
diff --git a/test/fixtures/server.ts b/test/fixtures/server.ts
@@ -15,7 +15,7 @@ const requestsPerSocket = Symbol('requestsPerSocket');
 export async function startServer(options?: {
   keepAliveTimeout?: number;
   https?: boolean;
-}): Promise<{ server: Server, url: string, closeServer: any }> {
+}): Promise<{ server: Server, url: string, urlWithDns: string, closeServer: any }> {
   let server: Server;
   const requestHandler = async (req: IncomingMessage, res: ServerResponse) => {
     const startTime = Date.now();
@@ -398,6 +398,7 @@ export async function startServer(options?: {
       const address: any = server.address();
       resolve({
         url: `${protocol}://localhost:${address.port}/`,
+        urlWithDns: `${protocol}://127.0.0.1:${address.port}/`,
         server,
         closeServer() {
           if (hasCloseAllConnections) {

diff --git a/test/head-request-should-keepalive.test.ts b/test/head-request-should-keepalive.test.ts
@@ -0,0 +1,97 @@
+import { strict as assert } from 'node:assert';
+import { scheduler } from 'node:timers/promises';
+import { describe, it, beforeAll, afterAll } from 'vitest';
+import { HttpClient } from '../src/index.js';
+import { startServer } from './fixtures/server.js';
+
+describe('head-request-should-keepalive.test.ts', () => {
+  // https://github.com/node-modules/urllib/issues/565
+  // head request should keepalive
+  let close: any;
+  let _url: string;
+  beforeAll(async () => {
+    const { closeServer, urlWithDns } = await startServer();
+    close = closeServer;
+    _url = urlWithDns;
+  });
+
+  afterAll(async () => {
+    await close();
+  });
+
+  it('should keepalive on GET => HEAD => HEAD Request', async () => {
+    const httpClient = new HttpClient();
+    let res = await httpClient.request(_url, {
+      method: 'GET',
+    });
+    assert.equal(res.status, 200);
+    // console.log(res.headers, res.res.socket);
+    assert.equal(res.headers.connection, 'keep-alive');
+    const socketId = res.res.socket.id;
+
+    await scheduler.wait(10);
+    res = await httpClient.request(_url, {
+      method: 'HEAD',
+    });
+    assert.equal(res.status, 200);
+    // console.log(res.headers, res.res.socket);
+    assert.equal(res.headers.connection, 'keep-alive');
+    assert.equal(res.res.socket.id, socketId);
+
+    await scheduler.wait(1);
+    res = await httpClient.request(_url, {
+      method: 'HEAD',
+    });
+    assert.equal(res.status, 200);
+    // console.log(res.headers, res.res.socket);
+    assert.equal(res.headers.connection, 'keep-alive');
+    assert.equal(res.res.socket.id, socketId);
+
+    res = await httpClient.request(_url, {
+      method: 'HEAD',
+    });
+    assert.equal(res.status, 200);
+    // console.log(res.headers, res.res.socket);
+    assert.equal(res.headers.connection, 'keep-alive');
+
+    await scheduler.wait(1);
+    res = await httpClient.request(_url, {
+      method: 'HEAD',
+    });
+    assert.equal(res.status, 200);
+    // console.log(res.headers, res.res.socket);
+    assert.equal(res.headers.connection, 'keep-alive');
+    assert.equal(res.res.socket.id, socketId);
+
+    await scheduler.wait(1);
+    res = await httpClient.request(_url, {
+      method: 'HEAD',
+    });
+    assert.equal(res.status, 200);
+    // console.log(res.headers, res.res.socket);
+    assert.equal(res.headers.connection, 'keep-alive');
+    assert.equal(res.res.socket.id, socketId);
+  });
+
+  it('should close connection when reset = true', async () => {
+    const httpClient = new HttpClient();
+    let res = await httpClient.request(_url, {
+      method: 'GET',
+      reset: true,
+    });
+    assert.equal(res.status, 200);
+    // console.log(res.headers, res.res.socket);
+    assert.equal(res.headers.connection, 'close');
+    const socketId = res.res.socket.id;
+
+    await scheduler.wait(10);
+    res = await httpClient.request(_url, {
+      method: 'HEAD',
+      reset: true,
+    });
+    assert.equal(res.status, 200);
+    // console.log(res.headers, res.res.socket);
+    assert.equal(res.headers.connection, 'close');
+    assert.notEqual(res.res.socket.id, socketId);
+  });
+});