fix: default clientError replies on reused connection (#4101) (#4133)

When fastify server receives request with invalid url
in a reused connection, it closes the connection
instead of 400 Bad Request reply.

The closed connection is then propagated by load balancer (ALB)
as 502 error. This turns client errors into closely monitored
server errors.

`socket.bytesWritten` is never going to be 0 on reused connection.

Co-authored-by: Branislav Katreniak <bkatreniak@slido.com>

fastify.js

@@ -621,7 +621,7 @@ function fastify (options) {
     // https://github.com/nodejs/node/blob/6ca23d7846cb47e84fd344543e394e50938540be/lib/_http_server.js#L666
 
     // If the socket is not writable, there is no reason to try to send data.
-    if (socket.writable && socket.bytesWritten === 0) {
+    if (socket.writable) {
       socket.write(`HTTP/1.1 400 Bad Request\r\nContent-Length: ${body.length}\r\nContent-Type: application/json\r\n\r\n${body}`)
     }
     socket.destroy(err)

test/request-error.test.js

@@ -150,7 +150,7 @@ test('default clientError handler ignores sockets in destroyed state', t => {
 })
 
 test('default clientError handler destroys sockets in writable state', t => {
-  t.plan(1)
+  t.plan(2)
 
   const fastify = Fastify({
     bodyLimit: 1,
@@ -166,6 +166,9 @@ test('default clientError handler destroys sockets in writable state', t => {
     },
     destroy () {
       t.pass('destroy should be called')
+    },
+    write (response) {
+      t.match(response, /^HTTP\/1.1 400 Bad Request/)
     }
   })
 })
@@ -186,6 +189,9 @@ test('default clientError handler destroys http sockets in non-writable state',
     },
     destroy () {
       t.pass('destroy should be called')
+    },
+    write (response) {
+      t.fail('write should not be called')
     }
   })
 })
@@ -270,3 +276,40 @@ test('encapsulated error handler binding', t => {
     t.equal(fastify.hello, undefined)
   })
 })
+
+test('default clientError replies with bad request on reused keep-alive connection', t => {
+  t.plan(2)
+
+  let response = ''
+
+  const fastify = Fastify({
+    bodyLimit: 1,
+    keepAliveTimeout: 100
+  })
+
+  fastify.get('/', (request, reply) => {
+    reply.send('OK\n')
+  })
+
+  fastify.listen({ port: 0 }, function (err) {
+    t.error(err)
+    fastify.server.unref()
+
+    const client = connect(fastify.server.address().port)
+
+    client.on('data', chunk => {
+      response += chunk.toString('utf-8')
+    })
+
+    client.on('end', () => {
+      t.match(response, /^HTTP\/1.1 200 OK.*HTTP\/1.1 400 Bad Request/s)
+    })
+
+    client.resume()
+    client.write('GET / HTTP/1.1\r\n')
+    client.write('\r\n\r\n')
+    client.write('GET /?a b HTTP/1.1\r\n')
+    client.write('Connection: close\r\n')
+    client.write('\r\n\r\n')
+  })
+})