Skip to content

Commit 3d8ec00

Browse files
trivikrtrivikr
authored
test: move cleanup from finally to after hooks (#5194)
* test: move cleanup from finally to after hooks Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com> * test: close server after client --------- Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com>
1 parent 21f64b1 commit 3d8ec00

7 files changed

Lines changed: 213 additions & 227 deletions

File tree

‎test/http2-dispatcher.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -239,8 +239,8 @@ test('Dispatcher#Upgrade - Should throw on non-websocket upgrade', async t => {
239239
allowH2: true
240240
})
241241

242-
after(() => server.close())
243242
after(() => client.close())
243+
after(() => server.close())
244244

245245
try {
246246
await client.upgrade({ path: '/', protocol: 'any' })
@@ -334,6 +334,8 @@ test('Dispatcher#Upgrade resumes queued requests after successful WebSocket upgr
334334
},
335335
allowH2: true
336336
})
337+
after(() => client.close())
338+
after(() => server.close())
337339

338340
let upgradeSocket
339341

@@ -355,8 +357,6 @@ test('Dispatcher#Upgrade resumes queued requests after successful WebSocket upgr
355357
} finally {
356358
upgradeSocket?.on('error', () => {})
357359
upgradeSocket?.end()
358-
await client.close()
359-
await new Promise((resolve) => server.close(resolve))
360360
}
361361

362362
await t.completed

‎test/interceptors/redirect.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -828,6 +828,8 @@ test('same-origin redirect preserves plain object headers with polluted Object.p
828828
res.end('redirected')
829829
}).listen(0)
830830

831+
after(() => server.close())
832+
831833
const originalIterator = Object.prototype[Symbol.iterator]
832834
// eslint-disable-next-line no-extend-native
833835
Object.prototype[Symbol.iterator] = function * () {}
@@ -851,7 +853,6 @@ test('same-origin redirect preserves plain object headers with polluted Object.p
851853
// eslint-disable-next-line no-extend-native
852854
Object.prototype[Symbol.iterator] = originalIterator
853855
}
854-
server.close()
855856
}
856857
})
857858

‎test/ip-prioritization.js‎

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
'use strict'
22

3-
const { test } = require('node:test')
3+
const { test, after } = require('node:test')
44
const { Client } = require('..')
55
const { createServer } = require('node:http')
66
const { once } = require('node:events')
@@ -30,21 +30,18 @@ test('HTTP/1.1 Request Prioritization', async (t) => {
3030
return socket
3131
}
3232
})
33+
after(() => client.close())
34+
after(() => server.close())
3335

34-
try {
35-
await client.request({
36-
path: '/',
37-
method: 'GET',
38-
typeOfService: 42
39-
})
36+
await client.request({
37+
path: '/',
38+
method: 'GET',
39+
typeOfService: 42
40+
})
4041

41-
// Check if priority was set
42-
if (priority !== 42) {
43-
throw new Error(`Expected priority 42, got ${priority}`)
44-
}
45-
} finally {
46-
await client.close()
47-
server.close()
42+
// Check if priority was set
43+
if (priority !== 42) {
44+
throw new Error(`Expected priority 42, got ${priority}`)
4845
}
4946
})
5047

‎test/pool-connection-error-memory-leak.js‎

Lines changed: 106 additions & 112 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
'use strict'
22

3-
const { test } = require('node:test')
3+
const { test, after } = require('node:test')
44
const assert = require('node:assert')
55
const { Pool } = require('..')
66
const { createServer } = require('node:http')
@@ -16,87 +16,84 @@ test('Pool client count does not grow on repeated connection errors', async (t)
1616
bodyTimeout: 100,
1717
headersTimeout: 100
1818
})
19+
after(() => pool.close())
20+
21+
const clientCounts = []
22+
23+
// Track initial client count
24+
clientCounts.push(pool[kClients].length)
25+
26+
// Make several requests that will fail with connection errors
27+
const requests = 5
28+
for (let i = 0; i < requests; i++) {
29+
try {
30+
await pool.request({
31+
path: `/${i}`,
32+
method: 'GET'
33+
})
34+
assert.fail('Request should have failed with a connection error')
35+
} catch (err) {
36+
// We expect connection errors, but the error might be wrapped
37+
assert.ok(
38+
err.code === 'ECONNREFUSED' ||
39+
err.cause?.code === 'ECONNREFUSED' ||
40+
err.code === 'UND_ERR_CONNECT',
41+
`Expected connection error but got: ${err.message} (${err.code})`
42+
)
43+
}
1944

20-
try {
21-
const clientCounts = []
22-
23-
// Track initial client count
45+
// Track client count after each request
2446
clientCounts.push(pool[kClients].length)
2547

26-
// Make several requests that will fail with connection errors
27-
const requests = 5
28-
for (let i = 0; i < requests; i++) {
29-
try {
30-
await pool.request({
31-
path: `/${i}`,
32-
method: 'GET'
33-
})
34-
assert.fail('Request should have failed with a connection error')
35-
} catch (err) {
36-
// We expect connection errors, but the error might be wrapped
37-
assert.ok(
38-
err.code === 'ECONNREFUSED' ||
39-
err.cause?.code === 'ECONNREFUSED' ||
40-
err.code === 'UND_ERR_CONNECT',
41-
`Expected connection error but got: ${err.message} (${err.code})`
42-
)
43-
}
44-
45-
// Track client count after each request
46-
clientCounts.push(pool[kClients].length)
47-
48-
// Small delay to allow for client cleanup
49-
await new Promise(resolve => setTimeout(resolve, 10))
50-
}
48+
// Small delay to allow for client cleanup
49+
await new Promise(resolve => setTimeout(resolve, 10))
50+
}
5151

52-
// The key test: verify that client count doesn't increase monotonically,
53-
// which would indicate the memory leak that was fixed
54-
const maxCount = Math.max(...clientCounts)
55-
assert.ok(
56-
clientCounts[clientCounts.length - 1] <= maxCount,
57-
`Client count should not increase continuously. Counts: ${clientCounts.join(', ')}`
58-
)
59-
60-
// Ensure the last two counts are similar (stabilized)
61-
const lastCount = clientCounts[clientCounts.length - 1]
62-
const secondLastCount = clientCounts[clientCounts.length - 2]
63-
64-
assert.ok(
65-
Math.abs(lastCount - secondLastCount) <= 1,
66-
`Client count should stabilize. Last counts: ${secondLastCount}, ${lastCount}`
67-
)
68-
69-
// Additional verification: make many more requests to check for significant growth
70-
const moreRequests = 10
71-
const startCount = pool[kClients].length
72-
73-
for (let i = 0; i < moreRequests; i++) {
74-
try {
75-
await pool.request({
76-
path: `/more-${i}`,
77-
method: 'GET'
78-
})
79-
} catch (err) {
80-
// Expected error
81-
}
82-
83-
// Small delay to allow for client cleanup
84-
await new Promise(resolve => setTimeout(resolve, 10))
52+
// The key test: verify that client count doesn't increase monotonically,
53+
// which would indicate the memory leak that was fixed
54+
const maxCount = Math.max(...clientCounts)
55+
assert.ok(
56+
clientCounts[clientCounts.length - 1] <= maxCount,
57+
`Client count should not increase continuously. Counts: ${clientCounts.join(', ')}`
58+
)
59+
60+
// Ensure the last two counts are similar (stabilized)
61+
const lastCount = clientCounts[clientCounts.length - 1]
62+
const secondLastCount = clientCounts[clientCounts.length - 2]
63+
64+
assert.ok(
65+
Math.abs(lastCount - secondLastCount) <= 1,
66+
`Client count should stabilize. Last counts: ${secondLastCount}, ${lastCount}`
67+
)
68+
69+
// Additional verification: make many more requests to check for significant growth
70+
const moreRequests = 10
71+
const startCount = pool[kClients].length
72+
73+
for (let i = 0; i < moreRequests; i++) {
74+
try {
75+
await pool.request({
76+
path: `/more-${i}`,
77+
method: 'GET'
78+
})
79+
} catch (err) {
80+
// Expected error
8581
}
8682

87-
const endCount = pool[kClients].length
83+
// Small delay to allow for client cleanup
84+
await new Promise(resolve => setTimeout(resolve, 10))
85+
}
8886

89-
// The maximum tolerable growth - some growth may occur due to timing issues,
90-
// but it should be limited and not proportional to the number of requests
91-
const maxGrowth = 3
87+
const endCount = pool[kClients].length
9288

93-
assert.ok(
94-
endCount - startCount <= maxGrowth,
95-
`Client count should not grow significantly after many failed requests. Start: ${startCount}, End: ${endCount}`
96-
)
97-
} finally {
98-
await pool.close()
99-
}
89+
// The maximum tolerable growth - some growth may occur due to timing issues,
90+
// but it should be limited and not proportional to the number of requests
91+
const maxGrowth = 3
92+
93+
assert.ok(
94+
endCount - startCount <= maxGrowth,
95+
`Client count should not grow significantly after many failed requests. Start: ${startCount}, End: ${endCount}`
96+
)
10097
})
10198

10299
// This test specifically verifies the fix in pool-base.js for connectionError event
@@ -113,46 +110,43 @@ test('Pool clients are removed on connectionError event', async (t) => {
113110
const pool = new Pool(`http://localhost:${port}`, {
114111
connections: 3 // Small pool to make testing easier
115112
})
113+
after(() => pool.close())
114+
after(() => server.close())
116115

117-
try {
118-
// Make an initial successful request to create a client
119-
await pool.request({
120-
path: '/',
121-
method: 'GET'
122-
})
123-
124-
// Save the initial number of clients
125-
const initialCount = pool[kClients].length
126-
assert.ok(initialCount > 0, 'Should have at least one client after a successful request')
116+
// Make an initial successful request to create a client
117+
await pool.request({
118+
path: '/',
119+
method: 'GET'
120+
})
127121

128-
// Manually trigger a connectionError on all clients
129-
for (const client of [...pool[kClients]]) {
130-
client.emit('connectionError', 'origin', [client], new Error('Simulated connection error'))
131-
}
122+
// Save the initial number of clients
123+
const initialCount = pool[kClients].length
124+
assert.ok(initialCount > 0, 'Should have at least one client after a successful request')
132125

133-
// Allow some time for the event to be processed
134-
await new Promise(resolve => setTimeout(resolve, 50))
135-
136-
// After the fix, all clients should be removed when they emit a connectionError
137-
assert.strictEqual(
138-
pool[kClients].length,
139-
0,
140-
'All clients should be removed from pool after connectionError events'
141-
)
142-
143-
// Make another request to verify the pool can create new clients
144-
await pool.request({
145-
path: '/after-error',
146-
method: 'GET'
147-
})
148-
149-
// Verify new clients were created
150-
assert.ok(
151-
pool[kClients].length > 0,
152-
'Pool should create new clients after previous ones were removed'
153-
)
154-
} finally {
155-
await pool.close()
156-
await new Promise(resolve => server.close(resolve))
126+
// Manually trigger a connectionError on all clients
127+
for (const client of [...pool[kClients]]) {
128+
client.emit('connectionError', 'origin', [client], new Error('Simulated connection error'))
157129
}
130+
131+
// Allow some time for the event to be processed
132+
await new Promise(resolve => setTimeout(resolve, 50))
133+
134+
// After the fix, all clients should be removed when they emit a connectionError
135+
assert.strictEqual(
136+
pool[kClients].length,
137+
0,
138+
'All clients should be removed from pool after connectionError events'
139+
)
140+
141+
// Make another request to verify the pool can create new clients
142+
await pool.request({
143+
path: '/after-error',
144+
method: 'GET'
145+
})
146+
147+
// Verify new clients were created
148+
assert.ok(
149+
pool[kClients].length > 0,
150+
'Pool should create new clients after previous ones were removed'
151+
)
158152
})

‎test/proxy-agent.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,8 @@ test('ProxyAgent forwards connectTimeout to the proxy connector', async (t) => {
144144
}
145145
})
146146

147+
after(() => proxyAgent.close())
148+
147149
try {
148150
net.connect = function (options) {
149151
return new net.Socket(options)
@@ -177,7 +179,6 @@ test('ProxyAgent forwards connectTimeout to the proxy connector', async (t) => {
177179
if (socket && !socket.destroyed) {
178180
socket.destroy()
179181
}
180-
await proxyAgent.close()
181182
}
182183
})
183184

‎test/request.js‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -447,6 +447,11 @@ describe('Should include headers from iterable objects', scope => {
447447
hello: 'world'
448448
}
449449

450+
after(() => {
451+
server.closeAllConnections?.()
452+
server.close()
453+
})
454+
450455
const originalIterator = Object.prototype[Symbol.iterator]
451456
// eslint-disable-next-line no-extend-native
452457
Object.prototype[Symbol.iterator] = function * () {}
@@ -472,8 +477,6 @@ describe('Should include headers from iterable objects', scope => {
472477
// eslint-disable-next-line no-extend-native
473478
Object.prototype[Symbol.iterator] = originalIterator
474479
}
475-
server.closeAllConnections?.()
476-
server.close()
477480
}
478481
})
479482

0 commit comments

Comments
 (0)