feat: ERR_REP_ALREADY_SENT hint that a route may be missing "return r…

…eply" (#4921)

* ERR_REP_ALREADY_SENT explicitate that a route is missing "return reply"

Signed-off-by: Matteo Collina <hello@matteocollina.com>

* fixup

Signed-off-by: Matteo Collina <hello@matteocollina.com>

* Apply suggestions from code review

Co-authored-by: Frazer Smith <frazer.dev@outlook.com>

* Add method

Signed-off-by: Matteo Collina <hello@matteocollina.com>

---------

Signed-off-by: Matteo Collina <hello@matteocollina.com>
Co-authored-by: Uzlopak <aras.abbasi@googlemail.com>
Co-authored-by: Frazer Smith <frazer.dev@outlook.com>

lib/errors.js

@@ -214,7 +214,7 @@ const codes = {
   ),
   FST_ERR_REP_ALREADY_SENT: createError(
     'FST_ERR_REP_ALREADY_SENT',
-    'Reply was already sent.'
+    'Reply was already sent, did you forget to "return reply" in "%s" (%s)?'
   ),
   FST_ERR_REP_SENT_VALUE: createError(
     'FST_ERR_REP_SENT_VALUE',

lib/reply.js

@@ -97,7 +97,7 @@ Object.defineProperties(Reply.prototype, {
 
       // We throw only if sent was overwritten from Fastify
       if (this.sent && this[kReplyHijacked]) {
-        throw new FST_ERR_REP_ALREADY_SENT()
+        throw new FST_ERR_REP_ALREADY_SENT(this.request.url, this.request.method)
       }
 
       this[kReplyHijacked] = true
@@ -124,7 +124,7 @@ Reply.prototype.send = function (payload) {
   }
 
   if (this.sent) {
-    this.log.warn({ err: new FST_ERR_REP_ALREADY_SENT() }, 'Reply already sent')
+    this.log.warn({ err: new FST_ERR_REP_ALREADY_SENT(this.request.url, this.request.method) })
     return this
   }
 
@@ -539,6 +539,18 @@ function wrapOnSendEnd (err, request, reply, payload) {
   }
 }
 
+function safeWriteHead (reply, statusCode) {
+  const res = reply.raw
+  try {
+    res.writeHead(statusCode, reply[kReplyHeaders])
+  } catch (err) {
+    if (err.code === 'ERR_HTTP_HEADERS_SENT') {
+      reply.log.warn(`Reply was already sent, did you forget to "return reply" in the "${reply.request.raw.url}" (${reply.request.raw.method}) route?`)
+    }
+    throw err
+  }
+}
+
 function onSendEnd (reply, payload) {
   const res = reply.raw
   const req = reply.request
@@ -569,7 +581,7 @@ function onSendEnd (reply, payload) {
       reply[kReplyHeaders]['content-length'] = '0'
     }
 
-    res.writeHead(statusCode, reply[kReplyHeaders])
+    safeWriteHead(reply, statusCode)
     sendTrailer(payload, res, reply)
     return
   }
@@ -594,7 +606,7 @@ function onSendEnd (reply, payload) {
     }
   }
 
-  res.writeHead(statusCode, reply[kReplyHeaders])
+  safeWriteHead(reply, statusCode)
   // write payload first
   res.write(payload)
   // then send trailers

test/async-await.test.js

@@ -124,7 +124,7 @@ test('ignore the result of the promise if reply.send is called beforehand (objec
 })
 
 test('server logs an error if reply.send is called and a value is returned via async/await', t => {
-  const lines = ['incoming request', 'request completed', 'Reply already sent']
+  const lines = ['incoming request', 'request completed', 'Reply was already sent, did you forget to "return reply" in "/" (GET)?']
   t.plan(lines.length + 2)
 
   const splitStream = split(JSON.parse)

test/internals/errors.test.js

@@ -339,10 +339,10 @@ test('FST_ERR_REP_INVALID_PAYLOAD_TYPE', t => {
 
 test('FST_ERR_REP_ALREADY_SENT', t => {
   t.plan(5)
-  const error = new errors.FST_ERR_REP_ALREADY_SENT()
+  const error = new errors.FST_ERR_REP_ALREADY_SENT('/hello', 'GET')
   t.equal(error.name, 'FastifyError')
   t.equal(error.code, 'FST_ERR_REP_ALREADY_SENT')
-  t.equal(error.message, 'Reply was already sent.')
+  t.equal(error.message, 'Reply was already sent, did you forget to "return reply" in "/hello" (GET)?')
   t.equal(error.statusCode, 500)
   t.ok(error instanceof Error)
 })

test/internals/reply.test.js

@@ -1508,7 +1508,7 @@ test('should throw error when passing falsy value to reply.sent', t => {
 })
 
 test('should throw error when attempting to set reply.sent more than once', t => {
-  t.plan(4)
+  t.plan(3)
   const fastify = Fastify()
 
   fastify.get('/', function (req, reply) {
@@ -1518,7 +1518,6 @@ test('should throw error when attempting to set reply.sent more than once', t =>
       t.fail('must throw')
     } catch (err) {
       t.equal(err.code, 'FST_ERR_REP_ALREADY_SENT')
-      t.equal(err.message, 'Reply was already sent.')
     }
     reply.raw.end()
   })
@@ -2088,3 +2087,35 @@ test('invalid response headers and custom error handler', async t => {
 
   await fastify.close()
 })
+
+test('reply.send will intercept ERR_HTTP_HEADERS_SENT and log an error message', t => {
+  t.plan(2)
+
+  const response = new Writable()
+  Object.assign(response, {
+    setHeader: () => {},
+    hasHeader: () => false,
+    getHeader: () => undefined,
+    writeHead: () => {
+      const err = new Error('kaboom')
+      err.code = 'ERR_HTTP_HEADERS_SENT'
+      throw err
+    },
+    write: () => {},
+    headersSent: true
+  })
+
+  const log = {
+    warn: (msg) => {
+      t.equal(msg, 'Reply was already sent, did you forget to "return reply" in the "/hello" (GET) route?')
+    }
+  }
+
+  const reply = new Reply(response, { [kRouteContext]: { onSend: null }, raw: { url: '/hello', method: 'GET' } }, log)
+
+  try {
+    reply.send('')
+  } catch (err) {
+    t.equal(err.code, 'ERR_HTTP_HEADERS_SENT')
+  }
+})

test/serial/logger.0.test.js

@@ -519,7 +519,12 @@ t.test('test log stream', (t) => {
   })
 
   t.test('reply.send logs an error if called twice in a row', async (t) => {
-    const lines = ['incoming request', 'request completed', 'Reply already sent', 'Reply already sent']
+    const lines = [
+      'incoming request',
+      'request completed',
+      'Reply was already sent, did you forget to "return reply" in "/" (GET)?',
+      'Reply was already sent, did you forget to "return reply" in "/" (GET)?'
+    ]
     t.plan(lines.length + 1)
 
     const stream = split(JSON.parse)