Skip to content

Commit 398c11a

Browse files
committed
Fix HTTP/2 timings NaN issue
When making HTTP/2 requests, `phases.request` was NaN because http-timer cannot capture connection timings from the proxied socket. Fixes #1958
1 parent 1c3a041 commit 398c11a

File tree

2 files changed

+30
-14
lines changed

2 files changed

+30
-14
lines changed

source/core/index.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -825,6 +825,16 @@ export default class Request extends Duplex implements RequestEvents<Request> {
825825
timings.phases.tcp = timings.connect - timings.socket;
826826
}
827827

828+
// Workaround for http-timer limitation with HTTP/2:
829+
// When using HTTP/2, the socket is a proxy that http-timer discards,
830+
// so lookup, connect, and secureConnect events are never captured.
831+
// This results in phases.request being NaN (undefined - undefined).
832+
// Set it to undefined to be consistent with other unavailable timings.
833+
// See https://github.com/sindresorhus/got/issues/1958
834+
if (timings && Number.isNaN(timings.phases.request)) {
835+
timings.phases.request = undefined;
836+
}
837+
828838
response.once('error', (error: Error) => {
829839
this._aborted = true;
830840

test/timings.ts

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import test from 'ava';
22
import got from '../source/index.js';
3-
import withServer, {withHttpsServer} from './helpers/with-server.js';
3+
import withServer from './helpers/with-server.js';
44

55
test('http/1 timings', withServer, async (t, server, got) => {
66
server.get('/', (_request, response) => {
@@ -28,32 +28,38 @@ test('http/1 timings', withServer, async (t, server, got) => {
2828
t.true(phases.total! >= 0);
2929
});
3030

31-
test('http/2 timings', withHttpsServer(), async (t, server, got) => {
32-
server.get('/', (_request, response) => {
33-
response.json({data: 'test'});
34-
});
35-
36-
const {timings} = await got({http2: true});
31+
test('http/2 timings', async t => {
32+
// Use a real HTTP/2 server (Google supports HTTP/2)
33+
const {timings} = await got('https://www.google.com/', {http2: true});
3734

35+
// These timings are available even for HTTP/2
3836
t.true(timings.start >= 0);
3937
t.true(timings.socket! >= 0);
40-
t.true(timings.lookup! >= 0);
41-
t.true(timings.connect! >= 0);
42-
t.true(timings.secureConnect! >= 0);
4338
t.true(timings.upload! >= 0);
4439
t.true(timings.response! >= 0);
4540
t.true(timings.end! >= 0);
4641

42+
// These connection timings are unavailable for HTTP/2 (socket is a proxy)
43+
// See https://github.com/sindresorhus/got/issues/1958
44+
t.is(timings.lookup, undefined);
45+
t.is(timings.connect, undefined);
46+
t.is(timings.secureConnect, undefined);
47+
4748
const {phases} = timings;
4849

50+
// Available phases
4951
t.true(phases.wait! >= 0);
50-
t.true(phases.dns! >= 0);
51-
t.true(phases.tcp! >= 0);
52-
t.true(phases.tls! >= 0);
53-
t.true(phases.request! >= 0);
5452
t.true(phases.firstByte! >= 0);
5553
t.true(phases.download! >= 0);
5654
t.true(phases.total! >= 0);
55+
56+
// Unavailable phases (due to missing connection timings)
57+
t.is(phases.dns, undefined);
58+
t.is(phases.tcp, undefined);
59+
t.is(phases.tls, undefined);
60+
// Most importantly: phases.request should be undefined, NOT NaN
61+
t.is(phases.request, undefined);
62+
t.false(Number.isNaN(phases.request));
5763
});
5864

5965
test('timings.end is set when stream is destroyed before completion', withServer, async (t, server, got) => {

0 commit comments

Comments
 (0)