test: add unexpected disconnect guards to more client test files

docs/docs/best-practices/writing-tests.md

@@ -18,3 +18,40 @@ const agent = new Agent({
 
 setGlobalDispatcher(agent)
 ```
+
+## Guarding against unexpected disconnects
+
+Undici's `Client` automatically reconnects after a socket error. This means
+a test can silently disconnect, reconnect, and still pass. Unfortunately, 
+this could mask bugs like unexpected parser errors or protocol violations.
+To catch these silent reconnections, use the `guardDisconnect` helper from
+`test/guard-disconnect.js` after creating a `Client` (or `Pool`):
+
+```js
+const { Client } = require('undici')
+const { test, after } = require('node:test')
+const { tspl } = require('@matteo.collina/tspl')
+const { guardDisconnect } = require('./guard-disconnect')
+
+test('example with disconnect guard', async (t) => {
+  t = tspl(t, { plan: 1 })
+
+  const client = new Client('http://localhost:3000')
+  after(() => client.close())
+
+  guardDisconnect(client, t)
+
+  // ... test logic ...
+})
+```
+
+The guard listens for `'disconnect'` events and calls `t.fail()` unless the
+client is already closing or destroyed. Skip the guard for tests where a disconnect is expected behavior, such as:
+
+- Signal aborts (`signal.emit('abort')`, `ac.abort()`)
+- Server-side destruction (`res.destroy()`, `req.socket.destroy()`)
+- Client-side body destruction mid-stream (`data.body.destroy()`)
+- Timeout errors (`HeadersTimeoutError`, `BodyTimeoutError`)
+- Successful upgrades (the socket is detached from the `Client`)
+- Retry/reconnect tests where the disconnect triggers the retry
+- HTTP parser errors from malformed responses (`HTTPParserError`)

test/client-connect.js

@@ -7,6 +7,7 @@ const { Client, errors } = require('..')
 const http = require('node:http')
 const EE = require('node:events')
 const { kBusy } = require('../lib/core/symbols')
+const { guardDisconnect } = require('./guard-disconnect')
 
 // TODO: move to test/node-test/client-connect.js
 test('connect aborted after connect', async (t) => {
@@ -29,11 +30,7 @@ test('connect aborted after connect', async (t) => {
     pipelining: 3
   })
   after(() => client.close())
-  client.on('disconnect', () => {
-    if (!client.closed && !client.destroyed) {
-      t.fail('unexpected disconnect')
-    }
-  })
+  guardDisconnect(client, t)
 
   client.connect({
     path: '/',

test/client-pipelining.js

@@ -9,6 +9,7 @@ const { kConnect } = require('../lib/core/symbols')
 const EE = require('node:events')
 const { kBusy, kRunning, kSize } = require('../lib/core/symbols')
 const { maybeWrapStream, consts } = require('./utils/async-iterators')
+const { guardDisconnect } = require('./guard-disconnect')
 
 test('20 times GET with pipelining 10', async (t) => {
   const num = 20
@@ -41,11 +42,7 @@ test('20 times GET with pipelining 10', async (t) => {
       pipelining: 10
     })
     after(() => client.close())
-    client.on('disconnect', () => {
-      if (!client.closed && !client.destroyed) {
-        t.fail('unexpected disconnect')
-      }
-    })
+    guardDisconnect(client, t)
 
     for (let i = 0; i < num; i++) {
       makeRequest(i)
@@ -103,11 +100,7 @@ test('A client should enqueue as much as twice its pipelining factor', async (t)
       pipelining: 2
     })
     after(() => client.close())
-    client.on('disconnect', () => {
-      if (!client.closed && !client.destroyed) {
-        t.fail('unexpected disconnect')
-      }
-    })
+    guardDisconnect(client, t)
 
     for (; sent < 2;) {
       t.ok(client[kSize] <= client.pipelining, 'client is not full')
@@ -158,11 +151,7 @@ test('pipeline 1 is 1 active request', async (t) => {
       pipelining: 1
     })
     after(() => client.destroy())
-    client.on('disconnect', () => {
-      if (!client.closed && !client.destroyed) {
-        t.fail('unexpected disconnect')
-      }
-    })
+    guardDisconnect(client, t)
     client.request({
       path: '/',
       method: 'GET'
@@ -216,11 +205,7 @@ test('pipelined chunked POST stream', async (t) => {
       pipelining: 2
     })
     after(() => client.close())
-    client.on('disconnect', () => {
-      if (!client.closed && !client.destroyed) {
-        t.fail('unexpected disconnect')
-      }
-    })
+    guardDisconnect(client, t)
 
     client.request({
       path: '/',
@@ -290,11 +275,7 @@ test('pipelined chunked POST iterator', async (t) => {
       pipelining: 2
     })
     after(() => client.close())
-    client.on('disconnect', () => {
-      if (!client.closed && !client.destroyed) {
-        t.fail('unexpected disconnect')
-      }
-    })
+    guardDisconnect(client, t)
 
     client.request({
       path: '/',
@@ -418,11 +399,7 @@ test('pipelining non-idempotent', async (t) => {
       pipelining: 2
     })
     after(() => client.close())
-    client.on('disconnect', () => {
-      if (!client.closed && !client.destroyed) {
-        t.fail('unexpected disconnect')
-      }
-    })
+    guardDisconnect(client, t)
 
     let ended = false
     client.request({
@@ -469,11 +446,7 @@ function pipeliningNonIdempotentWithBody (bodyType) {
         pipelining: 2
       })
       after(() => client.close())
-      client.on('disconnect', () => {
-        if (!client.closed && !client.destroyed) {
-          t.fail('unexpected disconnect')
-        }
-      })
+      guardDisconnect(client, t)
 
       let ended = false
       let reading = false
@@ -782,11 +755,7 @@ test('pipelining blocked', async (t) => {
       pipelining: 10
     })
     after(() => client.close())
-    client.on('disconnect', () => {
-      if (!client.closed && !client.destroyed) {
-        t.fail('unexpected disconnect')
-      }
-    })
+    guardDisconnect(client, t)
     client.request({
       path: '/',
       method: 'GET',

test/client-post.js

@@ -5,6 +5,7 @@ const { test, after } = require('node:test')
 const { once } = require('node:events')
 const { Client } = require('..')
 const { createServer } = require('node:http')
+const { guardDisconnect } = require('./guard-disconnect')
 
 test('request post blob', async (t) => {
   t = tspl(t, { plan: 3 })
@@ -26,11 +27,7 @@ test('request post blob', async (t) => {
 
   const client = new Client(`http://localhost:${server.address().port}`)
   after(client.destroy.bind(client))
-  client.on('disconnect', () => {
-    if (!client.closed && !client.destroyed) {
-      t.fail('unexpected disconnect')
-    }
-  })
+  guardDisconnect(client, t)
 
   client.request({
     path: '/',
@@ -67,11 +64,7 @@ test('request post arrayBuffer', async (t) => {
 
   const client = new Client(`http://localhost:${server.address().port}`)
   after(() => client.destroy())
-  client.on('disconnect', () => {
-    if (!client.closed && !client.destroyed) {
-      t.fail('unexpected disconnect')
-    }
-  })
+  guardDisconnect(client, t)
 
   const buf = Buffer.from('asd')
   const dst = new ArrayBuffer(buf.byteLength)

test/client-stream.js

@@ -6,6 +6,7 @@ const { Client, errors } = require('..')
 const { createServer } = require('node:http')
 const { PassThrough, Writable, Readable } = require('node:stream')
 const EE = require('node:events')
+const { guardDisconnect } = require('./guard-disconnect')
 
 test('stream get', async (t) => {
   t = tspl(t, { plan: 9 })
@@ -22,11 +23,7 @@ test('stream get', async (t) => {
   server.listen(0, () => {
     const client = new Client(`http://localhost:${server.address().port}`)
     after(() => client.close())
-    client.on('disconnect', () => {
-      if (!client.closed && !client.destroyed) {
-        t.fail('unexpected disconnect')
-      }
-    })
+    guardDisconnect(client, t)
 
     const signal = new EE()
     client.stream({
@@ -70,11 +67,7 @@ test('stream promise get', async (t) => {
   server.listen(0, async () => {
     const client = new Client(`http://localhost:${server.address().port}`)
     after(() => client.close())
-    client.on('disconnect', () => {
-      if (!client.closed && !client.destroyed) {
-        t.fail('unexpected disconnect')
-      }
-    })
+    guardDisconnect(client, t)
 
     await client.stream({
       path: '/',
@@ -264,11 +257,7 @@ test('stream waits only for writable side', async (t) => {
   server.listen(0, () => {
     const client = new Client(`http://localhost:${server.address().port}`)
     after(() => client.close())
-    client.on('disconnect', () => {
-      if (!client.closed && !client.destroyed) {
-        t.fail('unexpected disconnect')
-      }
-    })
+    guardDisconnect(client, t)
 
     const pt = new PassThrough({ autoDestroy: false })
     client.stream({
@@ -336,11 +325,7 @@ test('stream destroy if not readable', async (t) => {
   server.listen(0, () => {
     const client = new Client(`http://localhost:${server.address().port}`)
     after(client.destroy.bind(client))
-    client.on('disconnect', () => {
-      if (!client.closed && !client.destroyed) {
-        t.fail('unexpected disconnect')
-      }
-    })
+    guardDisconnect(client, t)
 
     client.stream({
       path: '/',
@@ -417,11 +402,7 @@ test('stream body without destroy', async (t) => {
   server.listen(0, () => {
     const client = new Client(`http://localhost:${server.address().port}`)
     after(client.destroy.bind(client))
-    client.on('disconnect', () => {
-      if (!client.closed && !client.destroyed) {
-        t.fail('unexpected disconnect')
-      }
-    })
+    guardDisconnect(client, t)
 
     client.stream({
       path: '/',
@@ -545,11 +526,7 @@ test('stream abort after complete', async (t) => {
   server.listen(0, () => {
     const client = new Client(`http://localhost:${server.address().port}`)
     after(client.destroy.bind(client))
-    client.on('disconnect', () => {
-      if (!client.closed && !client.destroyed) {
-        t.fail('unexpected disconnect')
-      }
-    })
+    guardDisconnect(client, t)
 
     const pt = new PassThrough()
     const signal = new EE()
@@ -610,11 +587,7 @@ test('trailers', async (t) => {
   server.listen(0, () => {
     const client = new Client(`http://localhost:${server.address().port}`)
     after(() => client.close())
-    client.on('disconnect', () => {
-      if (!client.closed && !client.destroyed) {
-        t.fail('unexpected disconnect')
-      }
-    })
+    guardDisconnect(client, t)
 
     client.stream({
       path: '/',
@@ -640,11 +613,7 @@ test('stream ignore 1xx', async (t) => {
   server.listen(0, () => {
     const client = new Client(`http://localhost:${server.address().port}`)
     after(() => client.close())
-    client.on('disconnect', () => {
-      if (!client.closed && !client.destroyed) {
-        t.fail('unexpected disconnect')
-      }
-    })
+    guardDisconnect(client, t)
 
     let buf = ''
     client.stream({
@@ -677,11 +646,7 @@ test('stream ignore 1xx and use onInfo', async (t) => {
   server.listen(0, () => {
     const client = new Client(`http://localhost:${server.address().port}`)
     after(() => client.close())
-    client.on('disconnect', () => {
-      if (!client.closed && !client.destroyed) {
-        t.fail('unexpected disconnect')
-      }
-    })
+    guardDisconnect(client, t)
 
     let buf = ''
     client.stream({
@@ -720,11 +685,7 @@ test('stream backpressure', async (t) => {
   server.listen(0, () => {
     const client = new Client(`http://localhost:${server.address().port}`)
     after(() => client.close())
-    client.on('disconnect', () => {
-      if (!client.closed && !client.destroyed) {
-        t.fail('unexpected disconnect')
-      }
-    })
+    guardDisconnect(client, t)
 
     let buf = ''
     client.stream({
@@ -786,11 +747,7 @@ test('stream needDrain', async (t) => {
     after(() => {
       client.destroy()
     })
-    client.on('disconnect', () => {
-      if (!client.closed && !client.destroyed) {
-        t.fail('unexpected disconnect')
-      }
-    })
+    guardDisconnect(client, t)
 
     const dst = new PassThrough()
     dst.pause()
@@ -847,11 +804,7 @@ test('stream legacy needDrain', async (t) => {
     after(() => {
       client.destroy()
     })
-    client.on('disconnect', () => {
-      if (!client.closed && !client.destroyed) {
-        t.fail('unexpected disconnect')
-      }
-    })
+    guardDisconnect(client, t)
 
     const dst = new PassThrough()
     dst.pause()

test/client-timeout.js

@@ -7,6 +7,7 @@ const { createServer } = require('node:http')
 const { Readable } = require('node:stream')
 const FakeTimers = require('@sinonjs/fake-timers')
 const timers = require('../lib/util/timers')
+const { guardDisconnect } = require('./guard-disconnect')
 
 test('refresh timeout on pause', async (t) => {
   t = tspl(t, { plan: 1 })
@@ -180,6 +181,8 @@ test('parser resume with no body timeout', async (t) => {
     })
     after(() => client.destroy())
 
+    guardDisconnect(client, t)
+
     client.dispatch({
       path: '/',
       method: 'GET'