Skip to content

Commit 54c4a2c

Browse files
fengmk2fengmk2
authored
fix: should HEAD request keepalive by default (#566)
closes #565 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Enhanced request handling with the addition of a `reset` property for more control over HTTP requests. - Improved diagnostics capabilities for socket management and error handling. - **Bug Fixes** - Updated error handling to ensure retries are based on specific error types. - **Tests** - Introduced new test cases to validate keep-alive behavior for HTTP HEAD requests. - Enhanced existing tests to ensure accurate tracking of socket requests and responses. - **Documentation** - Updated documentation for the `reset` property in request options to clarify its default behavior. - **Chores** - Minor adjustments to logging namespaces for consistency. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 26764a3 commit 54c4a2c

File tree

8 files changed

+223
-16
lines changed

8 files changed

+223
-16
lines changed

‎src/HttpClient.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -428,6 +428,7 @@ export class HttpClient extends EventEmitter {
428428
opaque: internalOpaque,
429429
dispatcher: args.dispatcher ?? this.#dispatcher,
430430
signal: args.signal,
431+
reset: false,
431432
};
432433
if (typeof args.highWaterMark === 'number') {
433434
requestOptions.highWaterMark = args.highWaterMark;

‎src/Request.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ export type RequestOptions = {
148148
* unix domain socket file path
149149
*/
150150
socketPath?: string | null;
151-
/** Whether the request should stablish a keep-alive or not. Default `undefined` */
151+
/** Whether the request should stablish a keep-alive or not. Default `false`, try to keep alive by default */
152152
reset?: boolean;
153153
/** Default: `64 KiB` */
154154
highWaterMark?: number;

‎src/diagnosticsChannel.ts‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,12 @@ import { performance } from 'node:perf_hooks';
33
import { debuglog } from 'node:util';
44
import { Socket } from 'node:net';
55
import { DiagnosticsChannel } from 'undici';
6+
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
7+
// @ts-ignore
68
import symbols from './symbols.js';
79
import { globalId, performanceTime } from './utils.js';
810

9-
const debug = debuglog('urllib:DiagnosticsChannel');
11+
const debug = debuglog('urllib/diagnosticsChannel');
1012
let initedDiagnosticsChannel = false;
1113
// https://undici.nodejs.org/#/docs/api/DiagnosticsChannel
1214
// client --> server
@@ -24,18 +26,29 @@ function subscribe(name: string, listener: (message: unknown, channelName: strin
2426
}
2527

2628
type SocketExtend = Socket & {
27-
[key: symbol]: string | number | Date | undefined;
29+
[key: symbol]: string | number | Date | undefined | boolean;
2830
};
2931

32+
let kSocketReset: symbol;
3033
function formatSocket(socket: SocketExtend) {
3134
if (!socket) return socket;
35+
if (!kSocketReset) {
36+
const symbols = Object.getOwnPropertySymbols(socket);
37+
for (const symbol of symbols) {
38+
if (symbol.description === 'reset') {
39+
kSocketReset = symbol;
40+
break;
41+
}
42+
}
43+
}
3244
return {
3345
localAddress: socket[symbols.kSocketLocalAddress],
3446
localPort: socket[symbols.kSocketLocalPort],
3547
remoteAddress: socket.remoteAddress,
3648
remotePort: socket.remotePort,
3749
attemptedAddresses: socket.autoSelectFamilyAttemptedAddresses,
3850
connecting: socket.connecting,
51+
reset: socket[kSocketReset],
3952
};
4053
}
4154

‎src/fetch.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ import { RawResponseWithMeta, SocketInfo } from './Response.js';
4242
import { IncomingHttpHeaders } from './IncomingHttpHeaders.js';
4343
import { BaseAgent, BaseAgentOptions } from './BaseAgent.js';
4444

45-
const debug = debuglog('urllib:fetch');
45+
const debug = debuglog('urllib/fetch');
4646

4747
export interface UrllibRequestInit extends RequestInit {
4848
// default is true

‎test/diagnostics_channel.test.ts‎

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ describe('diagnostics_channel.test.ts', () => {
4747
}
4848
}
4949
const handler = request[kHandler];
50+
if (!handler) return;
5051
let opaque = handler.opaque || handler.opts?.opaque;
5152
assert(opaque);
5253
opaque = opaque[symbols.kRequestOriginalOpaque];
@@ -88,7 +89,7 @@ describe('diagnostics_channel.test.ts', () => {
8889
assert(lastRequestOpaque.tracer.socket);
8990
assert.equal(lastRequestOpaque.tracer.socket.requests, 1);
9091

91-
// HEAD 请求不会 keepalive
92+
// HEAD 请求会走 keepalive
9293
// GET 请求会走 keepalive
9394
await sleep(1);
9495
traceId = `mock-traceid-${Date.now()}`;
@@ -102,7 +103,7 @@ describe('diagnostics_channel.test.ts', () => {
102103
assert.equal(response.status, 200);
103104
assert.equal(lastRequestOpaque.tracer.traceId, traceId);
104105
assert(lastRequestOpaque.tracer.socket);
105-
assert.equal(lastRequestOpaque.tracer.socket.requests, 1);
106+
assert.equal(lastRequestOpaque.tracer.socket.requests, 2);
106107

107108
await sleep(1);
108109
traceId = `mock-traceid-${Date.now()}`;
@@ -116,7 +117,7 @@ describe('diagnostics_channel.test.ts', () => {
116117
assert.equal(response.status, 200);
117118
assert.equal(lastRequestOpaque.tracer.traceId, traceId);
118119
assert(lastRequestOpaque.tracer.socket);
119-
assert.equal(lastRequestOpaque.tracer.socket.requests, 2);
120+
assert.equal(lastRequestOpaque.tracer.socket.requests, 3);
120121

121122
// socket 复用 1000 次
122123
let count = 1000;
@@ -133,7 +134,7 @@ describe('diagnostics_channel.test.ts', () => {
133134
assert.equal(response.status, 200);
134135
assert.equal(lastRequestOpaque.tracer.traceId, traceId);
135136
assert(lastRequestOpaque.tracer.socket);
136-
assert.equal(lastRequestOpaque.tracer.socket.requests, 2 + 1000 - count);
137+
assert.equal(lastRequestOpaque.tracer.socket.requests, 3 + 1000 - count);
137138
}
138139

139140
diagnosticsChannel.unsubscribe('undici:client:connected', onMessage);
@@ -311,7 +312,7 @@ describe('diagnostics_channel.test.ts', () => {
311312
assert.equal(socket.handledResponses, 1);
312313
assert.equal(lastRequestOpaque.tracer.traceId, traceId);
313314

314-
// HEAD 请求不会 keepalive
315+
// HEAD 请求会走 keepalive
315316
// GET 请求会走 keepalive
316317
await sleep(1);
317318
traceId = `mock-traceid-${Date.now()}`;
@@ -325,8 +326,8 @@ describe('diagnostics_channel.test.ts', () => {
325326
assert.equal(response.status, 200);
326327
assert.equal(lastRequestOpaque.tracer.traceId, traceId);
327328
assert(socket);
328-
assert.equal(socket.handledRequests, 1);
329-
assert.equal(socket.handledResponses, 1);
329+
assert.equal(socket.handledRequests, 2);
330+
assert.equal(socket.handledResponses, 2);
330331

331332
await sleep(1);
332333
traceId = `mock-traceid-${Date.now()}`;
@@ -340,8 +341,8 @@ describe('diagnostics_channel.test.ts', () => {
340341
assert.equal(response.status, 200);
341342
assert.equal(lastRequestOpaque.tracer.traceId, traceId);
342343
assert(socket);
343-
assert.equal(socket.handledRequests, 2);
344-
assert.equal(socket.handledResponses, 2);
344+
assert.equal(socket.handledRequests, 3);
345+
assert.equal(socket.handledResponses, 3);
345346

346347
// socket 复用 1000 次
347348
let count = 1000;
@@ -358,8 +359,8 @@ describe('diagnostics_channel.test.ts', () => {
358359
assert.equal(response.status, 200);
359360
assert.equal(lastRequestOpaque.tracer.traceId, traceId);
360361
assert(socket);
361-
assert.equal(socket.handledRequests, 2 + 1000 - count);
362-
assert.equal(socket.handledResponses, 2 + 1000 - count);
362+
assert.equal(socket.handledRequests, 3 + 1000 - count);
363+
assert.equal(socket.handledResponses, 3 + 1000 - count);
363364
}
364365

365366
diagnosticsChannel.unsubscribe('urllib:request', onRequestMessage);

‎test/fetch-head-request-should-keepalive.test.ts‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
import { strict as assert } from 'node:assert';
2+
import { scheduler } from 'node:timers/promises';
3+
import { describe, it, beforeAll, afterAll } from 'vitest';
4+
import { fetch } from '../src/index.js';
5+
import { startServer } from './fixtures/server.js';
6+
7+
describe.skip('fetch-head-request-should-keepalive.test.ts', () => {
8+
// https://github.com/node-modules/urllib/issues/565
9+
// head request should keepalive
10+
let close: any;
11+
let _url: string;
12+
beforeAll(async () => {
13+
const { closeServer, urlWithDns } = await startServer();
14+
close = closeServer;
15+
_url = urlWithDns;
16+
});
17+
18+
afterAll(async () => {
19+
await close();
20+
});
21+
22+
it('should keepalive on GET => HEAD => HEAD Request', async () => {
23+
let res = await fetch(_url, {
24+
method: 'GET',
25+
});
26+
assert.equal(res.status, 200);
27+
console.log(res.headers, res);
28+
assert.equal(res.headers.get('connection'), 'keep-alive');
29+
30+
await scheduler.wait(10);
31+
res = await fetch(_url, {
32+
method: 'HEAD',
33+
});
34+
assert.equal(res.status, 200);
35+
console.log(res.headers, res);
36+
assert.equal(res.headers.get('connection'), 'keep-alive');
37+
38+
// await scheduler.wait(1);
39+
// res = await httpClient.request(_url, {
40+
// method: 'HEAD',
41+
// });
42+
// assert.equal(res.status, 200);
43+
// // console.log(res.headers, res.res.socket);
44+
// assert.equal(res.headers.connection, 'keep-alive');
45+
// assert.equal(res.res.socket.id, socketId);
46+
47+
// res = await httpClient.request(_url, {
48+
// method: 'HEAD',
49+
// });
50+
// assert.equal(res.status, 200);
51+
// // console.log(res.headers, res.res.socket);
52+
// assert.equal(res.headers.connection, 'keep-alive');
53+
54+
// await scheduler.wait(1);
55+
// res = await httpClient.request(_url, {
56+
// method: 'HEAD',
57+
// });
58+
// assert.equal(res.status, 200);
59+
// // console.log(res.headers, res.res.socket);
60+
// assert.equal(res.headers.connection, 'keep-alive');
61+
// assert.equal(res.res.socket.id, socketId);
62+
63+
// await scheduler.wait(1);
64+
// res = await httpClient.request(_url, {
65+
// method: 'HEAD',
66+
// });
67+
// assert.equal(res.status, 200);
68+
// // console.log(res.headers, res.res.socket);
69+
// assert.equal(res.headers.connection, 'keep-alive');
70+
// assert.equal(res.res.socket.id, socketId);
71+
});
72+
73+
// it('should close connection when reset = true', async () => {
74+
// const httpClient = new HttpClient();
75+
// let res = await httpClient.request(_url, {
76+
// method: 'GET',
77+
// reset: true,
78+
// });
79+
// assert.equal(res.status, 200);
80+
// // console.log(res.headers, res.res.socket);
81+
// assert.equal(res.headers.connection, 'close');
82+
// const socketId = res.res.socket.id;
83+
84+
// await scheduler.wait(10);
85+
// res = await httpClient.request(_url, {
86+
// method: 'HEAD',
87+
// reset: true,
88+
// });
89+
// assert.equal(res.status, 200);
90+
// // console.log(res.headers, res.res.socket);
91+
// assert.equal(res.headers.connection, 'close');
92+
// assert.notEqual(res.res.socket.id, socketId);
93+
// });
94+
});

‎test/fixtures/server.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ const requestsPerSocket = Symbol('requestsPerSocket');
1515
export async function startServer(options?: {
1616
keepAliveTimeout?: number;
1717
https?: boolean;
18-
}): Promise<{ server: Server, url: string, closeServer: any }> {
18+
}): Promise<{ server: Server, url: string, urlWithDns: string, closeServer: any }> {
1919
let server: Server;
2020
const requestHandler = async (req: IncomingMessage, res: ServerResponse) => {
2121
const startTime = Date.now();
@@ -398,6 +398,7 @@ export async function startServer(options?: {
398398
const address: any = server.address();
399399
resolve({
400400
url: `${protocol}://localhost:${address.port}/`,
401+
urlWithDns: `${protocol}://127.0.0.1:${address.port}/`,
401402
server,
402403
closeServer() {
403404
if (hasCloseAllConnections) {

‎test/head-request-should-keepalive.test.ts‎

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
import { strict as assert } from 'node:assert';
2+
import { scheduler } from 'node:timers/promises';
3+
import { describe, it, beforeAll, afterAll } from 'vitest';
4+
import { HttpClient } from '../src/index.js';
5+
import { startServer } from './fixtures/server.js';
6+
7+
describe('head-request-should-keepalive.test.ts', () => {
8+
// https://github.com/node-modules/urllib/issues/565
9+
// head request should keepalive
10+
let close: any;
11+
let _url: string;
12+
beforeAll(async () => {
13+
const { closeServer, urlWithDns } = await startServer();
14+
close = closeServer;
15+
_url = urlWithDns;
16+
});
17+
18+
afterAll(async () => {
19+
await close();
20+
});
21+
22+
it('should keepalive on GET => HEAD => HEAD Request', async () => {
23+
const httpClient = new HttpClient();
24+
let res = await httpClient.request(_url, {
25+
method: 'GET',
26+
});
27+
assert.equal(res.status, 200);
28+
// console.log(res.headers, res.res.socket);
29+
assert.equal(res.headers.connection, 'keep-alive');
30+
const socketId = res.res.socket.id;
31+
32+
await scheduler.wait(10);
33+
res = await httpClient.request(_url, {
34+
method: 'HEAD',
35+
});
36+
assert.equal(res.status, 200);
37+
// console.log(res.headers, res.res.socket);
38+
assert.equal(res.headers.connection, 'keep-alive');
39+
assert.equal(res.res.socket.id, socketId);
40+
41+
await scheduler.wait(1);
42+
res = await httpClient.request(_url, {
43+
method: 'HEAD',
44+
});
45+
assert.equal(res.status, 200);
46+
// console.log(res.headers, res.res.socket);
47+
assert.equal(res.headers.connection, 'keep-alive');
48+
assert.equal(res.res.socket.id, socketId);
49+
50+
res = await httpClient.request(_url, {
51+
method: 'HEAD',
52+
});
53+
assert.equal(res.status, 200);
54+
// console.log(res.headers, res.res.socket);
55+
assert.equal(res.headers.connection, 'keep-alive');
56+
57+
await scheduler.wait(1);
58+
res = await httpClient.request(_url, {
59+
method: 'HEAD',
60+
});
61+
assert.equal(res.status, 200);
62+
// console.log(res.headers, res.res.socket);
63+
assert.equal(res.headers.connection, 'keep-alive');
64+
assert.equal(res.res.socket.id, socketId);
65+
66+
await scheduler.wait(1);
67+
res = await httpClient.request(_url, {
68+
method: 'HEAD',
69+
});
70+
assert.equal(res.status, 200);
71+
// console.log(res.headers, res.res.socket);
72+
assert.equal(res.headers.connection, 'keep-alive');
73+
assert.equal(res.res.socket.id, socketId);
74+
});
75+
76+
it('should close connection when reset = true', async () => {
77+
const httpClient = new HttpClient();
78+
let res = await httpClient.request(_url, {
79+
method: 'GET',
80+
reset: true,
81+
});
82+
assert.equal(res.status, 200);
83+
// console.log(res.headers, res.res.socket);
84+
assert.equal(res.headers.connection, 'close');
85+
const socketId = res.res.socket.id;
86+
87+
await scheduler.wait(10);
88+
res = await httpClient.request(_url, {
89+
method: 'HEAD',
90+
reset: true,
91+
});
92+
assert.equal(res.status, 200);
93+
// console.log(res.headers, res.res.socket);
94+
assert.equal(res.headers.connection, 'close');
95+
assert.notEqual(res.res.socket.id, socketId);
96+
});
97+
});

0 commit comments

Comments
 (0)