Skip to content

Commit fb03d84

Browse files
committed
Fix socket reuse timing phases preservation
We also inline a dependency so we can fix it. Fixes #2425
1 parent 142ed7c commit fb03d84

File tree

10 files changed

+738
-29
lines changed

10 files changed

+738
-29
lines changed

package.json

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,6 @@
5252
],
5353
"dependencies": {
5454
"@sindresorhus/is": "^7.0.1",
55-
"@szmarczak/http-timer": "^5.0.1",
5655
"byte-counter": "^0.1.0",
5756
"cacheable-lookup": "^7.0.0",
5857
"cacheable-request": "^13.0.12",

source/core/diagnostics-channel.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import {randomUUID} from 'node:crypto';
22
import diagnosticsChannel from 'node:diagnostics_channel';
3-
import type {Timings} from '@szmarczak/http-timer';
3+
import type {Timings} from './utils/timer.js';
44
import type {RequestError} from './errors.js';
55

66
const channels = {

source/core/errors.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import is from '@sindresorhus/is';
2-
import type {Timings} from '@szmarczak/http-timer';
2+
import type {Timings} from './utils/timer.js';
33
import type Options from './options.js';
44
import type {TimeoutError as TimedOutTimeoutError} from './timed-out.js';
55
import type {PlainResponse, Response} from './response.js';

source/core/index.ts

Lines changed: 1 addition & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import {Duplex, type Readable} from 'node:stream';
44
import http, {ServerResponse, type ClientRequest, type RequestOptions} from 'node:http';
55
import type {Socket} from 'node:net';
66
import {byteLength} from 'byte-counter';
7-
import timer, {type ClientRequestWithTimings, type Timings, type IncomingMessageWithTimings} from '@szmarczak/http-timer';
87
import CacheableRequest, {
98
CacheError as CacheableCacheError,
109
type CacheableRequestFunction,
@@ -16,6 +15,7 @@ import type KeyvType from 'keyv';
1615
import is, {isBuffer} from '@sindresorhus/is';
1716
import {FormDataEncoder, isFormData as isFormDataLike} from 'form-data-encoder';
1817
import type ResponseLike from 'responselike';
18+
import timer, {type ClientRequestWithTimings, type Timings, type IncomingMessageWithTimings} from './utils/timer.js';
1919
import getBodySize from './utils/get-body-size.js';
2020
import isFormData from './utils/is-form-data.js';
2121
import proxyEvents from './utils/proxy-events.js';
@@ -828,29 +828,6 @@ export default class Request extends Duplex implements RequestEvents<Request> {
828828
isFromCache: typedResponse.isFromCache,
829829
});
830830

831-
// Workaround for http-timer bug: when connecting to an IP address (no DNS lookup),
832-
// http-timer sets lookup = connect instead of lookup = socket, resulting in
833-
// dns = lookup - socket being a small positive number instead of 0.
834-
// See https://github.com/sindresorhus/got/issues/2279
835-
const {timings} = response;
836-
if (timings?.lookup !== undefined && timings.socket !== undefined && timings.connect !== undefined && timings.lookup === timings.connect && timings.phases.dns !== 0) {
837-
// Fix the DNS phase to be 0 and set lookup to socket time
838-
timings.phases.dns = 0;
839-
timings.lookup = timings.socket;
840-
// Recalculate TCP time to be the full time from socket to connect
841-
timings.phases.tcp = timings.connect - timings.socket;
842-
}
843-
844-
// Workaround for http-timer limitation with HTTP/2:
845-
// When using HTTP/2, the socket is a proxy that http-timer discards,
846-
// so lookup, connect, and secureConnect events are never captured.
847-
// This results in phases.request being NaN (undefined - undefined).
848-
// Set it to undefined to be consistent with other unavailable timings.
849-
// See https://github.com/sindresorhus/got/issues/1958
850-
if (timings && Number.isNaN(timings.phases.request)) {
851-
timings.phases.request = undefined;
852-
}
853-
854831
response.once('error', (error: Error) => {
855832
this._aborted = true;
856833

source/core/options.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,8 @@ import {isFormData, type FormDataLike} from 'form-data-encoder';
2121
import type {KeyvStoreAdapter} from 'keyv';
2222
import type KeyvType from 'keyv';
2323
import type ResponseLike from 'responselike';
24-
import type {IncomingMessageWithTimings} from '@szmarczak/http-timer';
2524
import type {CancelableRequest} from '../as-promise/types.js';
25+
import type {IncomingMessageWithTimings} from './utils/timer.js';
2626
import parseLinkHeader from './parse-link-header.js';
2727
import type {PlainResponse, Response} from './response.js';
2828
import type {RequestError} from './errors.js';

source/core/response.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import type {Buffer} from 'node:buffer';
2-
import type {IncomingMessageWithTimings, Timings} from '@szmarczak/http-timer';
2+
import type {IncomingMessageWithTimings, Timings} from './utils/timer.js';
33
import {RequestError} from './errors.js';
44
import type {ParseJsonFunction, ResponseType} from './options.js';
55
import type Request from './index.js';
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
import type {Socket} from 'node:net';
2+
import type {TLSSocket} from 'node:tls';
3+
4+
type Listeners = {
5+
connect?: () => void;
6+
secureConnect?: () => void;
7+
close?: (hadError: boolean) => void;
8+
};
9+
10+
function isTlsSocket(socket: Socket | TLSSocket): socket is TLSSocket {
11+
return 'encrypted' in socket;
12+
}
13+
14+
const deferToConnect = (socket: TLSSocket | Socket, fn: Listeners | (() => void)): void => {
15+
let listeners: Listeners;
16+
if (typeof fn === 'function') {
17+
const connect = fn;
18+
listeners = {connect};
19+
} else {
20+
listeners = fn;
21+
}
22+
23+
const hasConnectListener = typeof listeners.connect === 'function';
24+
const hasSecureConnectListener = typeof listeners.secureConnect === 'function';
25+
const hasCloseListener = typeof listeners.close === 'function';
26+
27+
const onConnect = () => {
28+
if (hasConnectListener) {
29+
listeners.connect!();
30+
}
31+
32+
if (isTlsSocket(socket) && hasSecureConnectListener) {
33+
if (socket.authorized) {
34+
listeners.secureConnect!();
35+
} else {
36+
// Wait for secureConnect event (even if authorization fails, we need the timing)
37+
socket.once('secureConnect', listeners.secureConnect!);
38+
}
39+
}
40+
41+
if (hasCloseListener) {
42+
socket.once('close', listeners.close!);
43+
}
44+
};
45+
46+
if (socket.writable && !socket.connecting) {
47+
onConnect();
48+
} else if (socket.connecting) {
49+
socket.once('connect', onConnect);
50+
} else if (socket.destroyed && hasCloseListener) {
51+
const hadError = '_hadError' in socket ? Boolean((socket as any)._hadError) : false;
52+
listeners.close!(hadError);
53+
}
54+
};
55+
56+
export default deferToConnect;

source/core/utils/timer.ts

Lines changed: 225 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,225 @@
1+
import {errorMonitor} from 'node:events';
2+
import {types} from 'node:util';
3+
import type {ClientRequest, IncomingMessage} from 'node:http';
4+
import type {Socket} from 'node:net';
5+
import deferToConnect from './defer-to-connect.js';
6+
7+
export type Timings = {
8+
start: number;
9+
socket?: number;
10+
lookup?: number;
11+
connect?: number;
12+
secureConnect?: number;
13+
upload?: number;
14+
response?: number;
15+
end?: number;
16+
error?: number;
17+
abort?: number;
18+
phases: {
19+
wait?: number;
20+
dns?: number;
21+
tcp?: number;
22+
tls?: number;
23+
request?: number;
24+
firstByte?: number;
25+
download?: number;
26+
total?: number;
27+
};
28+
};
29+
30+
export type ClientRequestWithTimings = ClientRequest & {
31+
timings?: Timings;
32+
};
33+
34+
export type IncomingMessageWithTimings = IncomingMessage & {
35+
timings?: Timings;
36+
};
37+
38+
type SocketWithConnectionTimings = Socket & {
39+
__initial_connection_timings__?: {
40+
dnsPhase: number;
41+
tcpPhase: number;
42+
tlsPhase?: number;
43+
};
44+
};
45+
46+
const timer = (request: ClientRequestWithTimings): Timings => {
47+
if (request.timings) {
48+
return request.timings;
49+
}
50+
51+
const timings: Timings = {
52+
start: Date.now(),
53+
socket: undefined,
54+
lookup: undefined,
55+
connect: undefined,
56+
secureConnect: undefined,
57+
upload: undefined,
58+
response: undefined,
59+
end: undefined,
60+
error: undefined,
61+
abort: undefined,
62+
phases: {
63+
wait: undefined,
64+
dns: undefined,
65+
tcp: undefined,
66+
tls: undefined,
67+
request: undefined,
68+
firstByte: undefined,
69+
download: undefined,
70+
total: undefined,
71+
},
72+
};
73+
74+
request.timings = timings;
75+
76+
const handleError = (origin: ClientRequest | IncomingMessage) => {
77+
origin.once(errorMonitor, () => {
78+
timings.error = Date.now();
79+
timings.phases.total = timings.error - timings.start;
80+
});
81+
};
82+
83+
handleError(request);
84+
85+
const onAbort = () => {
86+
timings.abort = Date.now();
87+
timings.phases.total = timings.abort - timings.start;
88+
};
89+
90+
request.prependOnceListener('abort', onAbort);
91+
92+
const onSocket = (socket: SocketWithConnectionTimings) => {
93+
timings.socket = Date.now();
94+
timings.phases.wait = timings.socket - timings.start;
95+
96+
if (types.isProxy(socket)) {
97+
// HTTP/2: The socket is a proxy, so connection events won't fire.
98+
// We can't measure connection timings, so leave them undefined.
99+
// This prevents NaN in phases.request calculation.
100+
return;
101+
}
102+
103+
// Check if socket is already connected (reused from connection pool)
104+
const socketAlreadyConnected = socket.writable && !socket.connecting;
105+
106+
if (socketAlreadyConnected) {
107+
// Socket reuse detected: the socket was already connected from a previous request.
108+
// For reused sockets, set all connection timestamps to socket time since no new
109+
// connection was made for THIS request. But preserve phase durations from the
110+
// original connection so they're not lost.
111+
timings.lookup = timings.socket;
112+
timings.connect = timings.socket;
113+
114+
if (socket.__initial_connection_timings__) {
115+
// Restore the phase timings from the initial connection
116+
timings.phases.dns = socket.__initial_connection_timings__.dnsPhase;
117+
timings.phases.tcp = socket.__initial_connection_timings__.tcpPhase;
118+
timings.phases.tls = socket.__initial_connection_timings__.tlsPhase;
119+
120+
// Set secureConnect timestamp if there was TLS
121+
if (timings.phases.tls !== undefined) {
122+
timings.secureConnect = timings.socket;
123+
}
124+
} else {
125+
// Socket reused but no initial timings stored (e.g., from external code)
126+
// Set phases to 0
127+
timings.phases.dns = 0;
128+
timings.phases.tcp = 0;
129+
}
130+
131+
return;
132+
}
133+
134+
const lookupListener = () => {
135+
timings.lookup = Date.now();
136+
timings.phases.dns = timings.lookup - timings.socket!;
137+
};
138+
139+
socket.prependOnceListener('lookup', lookupListener);
140+
141+
deferToConnect(socket, {
142+
connect() {
143+
timings.connect = Date.now();
144+
if (timings.lookup === undefined) {
145+
// No DNS lookup occurred (e.g., connecting to an IP address directly)
146+
// Set lookup to socket time (no time elapsed for DNS)
147+
socket.removeListener('lookup', lookupListener);
148+
timings.lookup = timings.socket!;
149+
timings.phases.dns = 0;
150+
}
151+
152+
timings.phases.tcp = timings.connect - timings.lookup;
153+
154+
// Store connection phase timings on socket for potential reuse
155+
if (!socket.__initial_connection_timings__) {
156+
socket.__initial_connection_timings__ = {
157+
dnsPhase: timings.phases.dns!,
158+
// eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion -- TypeScript can't prove this is defined due to callback structure
159+
tcpPhase: timings.phases.tcp!,
160+
};
161+
}
162+
},
163+
secureConnect() {
164+
timings.secureConnect = Date.now();
165+
timings.phases.tls = timings.secureConnect - timings.connect!;
166+
167+
// Update stored timings with TLS phase timing
168+
if (socket.__initial_connection_timings__) {
169+
socket.__initial_connection_timings__.tlsPhase = timings.phases.tls;
170+
}
171+
},
172+
});
173+
};
174+
175+
if (request.socket) {
176+
onSocket(request.socket as SocketWithConnectionTimings);
177+
} else {
178+
request.prependOnceListener('socket', onSocket as (socket: Socket) => void);
179+
}
180+
181+
const onUpload = () => {
182+
timings.upload = Date.now();
183+
184+
// Calculate request phase if we have connection timings
185+
const secureOrConnect = timings.secureConnect ?? timings.connect;
186+
if (secureOrConnect !== undefined) {
187+
timings.phases.request = timings.upload - secureOrConnect;
188+
}
189+
// If both are undefined (HTTP/2), phases.request stays undefined (not NaN)
190+
};
191+
192+
if (request.writableFinished) {
193+
onUpload();
194+
} else {
195+
request.prependOnceListener('finish', onUpload);
196+
}
197+
198+
request.prependOnceListener('response', (response: IncomingMessageWithTimings) => {
199+
timings.response = Date.now();
200+
timings.phases.firstByte = timings.response - timings.upload!;
201+
202+
response.timings = timings;
203+
handleError(response);
204+
205+
response.prependOnceListener('end', () => {
206+
request.off('abort', onAbort);
207+
response.off('aborted', onAbort);
208+
209+
if (timings.phases.total !== undefined) {
210+
// Aborted or errored
211+
return;
212+
}
213+
214+
timings.end = Date.now();
215+
timings.phases.download = timings.end - timings.response!;
216+
timings.phases.total = timings.end - timings.start;
217+
});
218+
219+
response.prependOnceListener('aborted', onAbort);
220+
});
221+
222+
return timings;
223+
};
224+
225+
export default timer;

0 commit comments

Comments
 (0)