Skip to content

Commit 61d5e3b

Browse files
committed
Fix DNS cache timing confusion
szmarczak/http-timer#35
1 parent fb03d84 commit 61d5e3b

File tree

2 files changed

+56
-0
lines changed

2 files changed

+56
-0
lines changed

source/core/utils/timer.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,14 @@ const timer = (request: ClientRequestWithTimings): Timings => {
151151

152152
timings.phases.tcp = timings.connect - timings.lookup;
153153

154+
// If lookup and connect happen at the EXACT same time (tcp = 0),
155+
// DNS was served from cache and the dns value is just event loop overhead.
156+
// Set dns to 0 to indicate no actual DNS resolution occurred.
157+
// Fixes https://github.com/szmarczak/http-timer/issues/35
158+
if (timings.phases.tcp === 0 && timings.phases.dns && timings.phases.dns > 0) {
159+
timings.phases.dns = 0;
160+
}
161+
154162
// Store connection phase timings on socket for potential reuse
155163
if (!socket.__initial_connection_timings__) {
156164
socket.__initial_connection_timings__ = {

test/timings.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import http from 'node:http';
12
import test from 'ava';
23
import got from '../source/index.js';
34
import withServer from './helpers/with-server.js';
@@ -109,6 +110,53 @@ test('dns timing is 0 for IP addresses', withServer, async (t, server) => {
109110
t.is(timings.lookup, timings.socket);
110111
});
111112

113+
test('dns timing is 0 for cached DNS lookups', withServer, async (t, server, got) => {
114+
server.get('/', (_request, response) => {
115+
response.end('ok');
116+
});
117+
118+
// Enable DNS cache and disable keep-alive to get new connections
119+
const instance = got.extend({
120+
dnsCache: true,
121+
agent: {
122+
http: new http.Agent({
123+
keepAlive: false,
124+
}),
125+
},
126+
});
127+
128+
// First request: real DNS lookup
129+
const response1 = await instance('');
130+
const firstDns = response1.timings.phases.dns;
131+
132+
// First request should have some DNS time (for localhost lookup)
133+
// or 0 if it's fast enough to trigger the cache threshold
134+
t.true(firstDns! >= 0);
135+
136+
// Subsequent requests: DNS should be cached
137+
const response2 = await instance('');
138+
const response3 = await instance('');
139+
140+
// When DNS is cached, if lookup and connect happen at the exact same time (tcp=0),
141+
// then dns is set to 0 to indicate no actual DNS resolution occurred.
142+
// Otherwise, dns will be small but may vary on CI due to system load.
143+
// The key fix from http-timer #35 is that we handle this case, not enforce exact values.
144+
const secondIsInstant = response2.timings.phases.tcp === 0;
145+
const thirdIsInstant = response3.timings.phases.tcp === 0;
146+
147+
if (secondIsInstant) {
148+
t.is(response2.timings.phases.dns, 0, 'instant cached DNS (tcp=0) should have dns=0');
149+
} else {
150+
t.true(response2.timings.phases.dns! >= 0, 'cached DNS should have dns >= 0');
151+
}
152+
153+
if (thirdIsInstant) {
154+
t.is(response3.timings.phases.dns, 0, 'instant cached DNS (tcp=0) should have dns=0');
155+
} else {
156+
t.true(response3.timings.phases.dns! >= 0, 'cached DNS should have dns >= 0');
157+
}
158+
});
159+
112160
test('redirect timings preserve connection timings from initial request', withServer, async (t, server, got) => {
113161
// Set up a redirect chain to test socket reuse scenario
114162
server.get('/', (_request, response) => {

0 commit comments

Comments
 (0)