From 68f5713300abedef7f04e4b51f935f742b6167b5 Mon Sep 17 00:00:00 2001 From: fengmk2 Date: Thu, 19 Dec 2024 15:57:12 +0800 Subject: [PATCH] fix: should HEAD request keepalive by default closes https://github.com/node-modules/urllib/issues/565 --- src/HttpClient.ts | 1 + src/Request.ts | 2 +- src/diagnosticsChannel.ts | 17 +++- src/fetch.ts | 2 +- test/diagnostics_channel.test.ts | 23 ++--- ...etch-head-request-should-keepalive.test.ts | 94 ++++++++++++++++++ test/fixtures/server.ts | 3 +- test/head-request-should-keepalive.test.ts | 97 +++++++++++++++++++ 8 files changed, 223 insertions(+), 16 deletions(-) create mode 100644 test/fetch-head-request-should-keepalive.test.ts create mode 100644 test/head-request-should-keepalive.test.ts diff --git a/src/HttpClient.ts b/src/HttpClient.ts index e1123230..d913cdad 100644 --- 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 index 3f83c44b..bb776167 100644 --- 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 index 3529f3df..323d4c46 100644 --- 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,11 +26,21 @@ 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], @@ -36,6 +48,7 @@ function formatSocket(socket: SocketExtend) { remotePort: socket.remotePort, attemptedAddresses: socket.autoSelectFamilyAttemptedAddresses, connecting: socket.connecting, + reset: socket[kSocketReset], }; } diff --git a/src/fetch.ts b/src/fetch.ts index 8b225bc7..7a7f09f5 100644 --- 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 index fa00569d..9bedee67 100644 --- 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 new file mode 100644 index 00000000..824b1372 --- /dev/null +++ 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 index 47961dad..055f2fae 100644 --- 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 new file mode 100644 index 00000000..8472324f --- /dev/null +++ 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); + }); +});