Simplify reply sent monitoring

lib/reply.js

@@ -8,7 +8,6 @@ const {
   kSchemaResponse,
   kFourOhFourContext,
   kReplyErrorHandlerCalled,
-  kReplySent,
   kReplySentOverwritten,
   kReplyStartTime,
   kReplySerializer,
@@ -52,7 +51,6 @@ const warning = require('./warnings')
 
 function Reply (res, request, log) {
   this.raw = res
-  this[kReplySent] = false
   this[kReplySerializer] = null
   this[kReplyErrorHandlerCalled] = false
   this[kReplyIsError] = false
@@ -79,19 +77,24 @@ Object.defineProperties(Reply.prototype, {
   sent: {
     enumerable: true,
     get () {
-      return this[kReplySent]
+      // We are checking whether reply was manually marked as sent or the
+      // response has ended. The response.writableEnded property was added in
+      // Node.js v12.9.0. Since fastify supports older Node.js versions as well,
+      // we have to take response.finished property in consideration when
+      // applicable.
+      // TODO: remove fallback when the lowest supported Node.js version >= v12.9.0
+      return (this[kReplySentOverwritten] || (typeof this.raw.writableEnded !== 'undefined' ? this.raw.writableEnded : this.raw.finished)) === true
     },
     set (value) {
       if (value !== true) {
         throw new FST_ERR_REP_SENT_VALUE()
       }
 
-      if (this[kReplySent]) {
+      if (this.sent) {
         throw new FST_ERR_REP_ALREADY_SENT()
       }
 
       this[kReplySentOverwritten] = true
-      this[kReplySent] = true
     }
   },
   statusCode: {
@@ -105,7 +108,7 @@ Object.defineProperties(Reply.prototype, {
 })
 
 Reply.prototype.hijack = function () {
-  this[kReplySent] = true
+  this[kReplySentOverwritten] = true
   return this
 }
 
@@ -114,7 +117,7 @@ Reply.prototype.send = function (payload) {
     throw new FST_ERR_SEND_INSIDE_ONERR()
   }
 
-  if (this[kReplySent]) {
+  if (this.sent) {
     this.log.warn({ err: new FST_ERR_REP_ALREADY_SENT() }, 'Reply already sent')
     return this
   }
@@ -373,7 +376,6 @@ function preserializeHookEnd (err, request, reply, payload) {
 }
 
 function onSendHook (reply, payload) {
-  reply[kReplySent] = true
   if (reply.context.onSend !== null) {
     onSendHookRunner(
       reply.context.onSend,
@@ -401,8 +403,6 @@ function onSendEnd (reply, payload) {
   const statusCode = res.statusCode
 
   if (payload === undefined || payload === null) {
-    reply[kReplySent] = true
-
     // according to https://tools.ietf.org/html/rfc7230#section-3.3.2
     // we cannot send a content-length for 304 and 204, and all status code
     // < 200.
@@ -432,8 +432,6 @@ function onSendEnd (reply, payload) {
     reply[kReplyHeaders]['content-length'] = '' + Buffer.byteLength(payload)
   }
 
-  reply[kReplySent] = true
-
   res.writeHead(statusCode, reply[kReplyHeaders])
 
   // avoid ArgumentsAdaptorTrampoline from V8
@@ -503,7 +501,6 @@ function sendStream (payload, res, reply) {
 }
 
 function onErrorHook (reply, error, cb) {
-  reply[kReplySent] = true
   if (reply.context.onError !== null && reply[kReplyErrorHandlerCalled] === true) {
     reply[kReplyIsRunningOnErrorHook] = true
     onSendHookRunner(
@@ -539,7 +536,6 @@ function handleError (reply, error, cb) {
 
   const errorHandler = reply.context.errorHandler
   if (errorHandler && reply[kReplyErrorHandlerCalled] === false) {
-    reply[kReplySent] = false
     reply[kReplyIsError] = false
     reply[kReplyErrorHandlerCalled] = true
     reply[kReplyHeaders]['content-length'] = undefined
@@ -585,7 +581,6 @@ function handleError (reply, error, cb) {
     return
   }
 
-  reply[kReplySent] = true
   res.writeHead(res.statusCode, reply[kReplyHeaders])
   res.end(payload)
 }
@@ -647,7 +642,6 @@ function buildReply (R) {
     this.raw = res
     this[kReplyIsError] = false
     this[kReplyErrorHandlerCalled] = false
-    this[kReplySent] = false
     this[kReplySentOverwritten] = false
     this[kReplySerializer] = null
     this.request = request
@@ -660,7 +654,6 @@ function buildReply (R) {
 }
 
 function notFound (reply) {
-  reply[kReplySent] = false
   reply[kReplyIsError] = false
 
   if (reply.context[kFourOhFourContext] === null) {

lib/symbols.js

@@ -30,7 +30,6 @@ const keys = {
   kReplyIsError: Symbol('fastify.reply.isError'),
   kReplyHeaders: Symbol('fastify.reply.headers'),
   kReplyHasStatusCode: Symbol('fastify.reply.hasStatusCode'),
-  kReplySent: Symbol('fastify.reply.sent'),
   kReplySentOverwritten: Symbol('fastify.reply.sentOverwritten'),
   kReplyStartTime: Symbol('fastify.reply.startTime'),
   kReplyErrorHandlerCalled: Symbol('fastify.reply.errorHandlerCalled'),

lib/wrapThenable.js

@@ -2,7 +2,6 @@
 
 const {
   kReplyIsError,
-  kReplySent,
   kReplySentOverwritten
 } = require('./symbols')
 
@@ -16,26 +15,24 @@ function wrapThenable (thenable, reply) {
 
     // this is for async functions that
     // are using reply.send directly
-    if (payload !== undefined || (reply.raw.statusCode === 204 && reply[kReplySent] === false)) {
+    if (payload !== undefined || (reply.raw.statusCode === 204 && reply.sent === false)) {
       // we use a try-catch internally to avoid adding a catch to another
       // promise, increase promise perf by 10%
       try {
         reply.send(payload)
       } catch (err) {
-        reply[kReplySent] = false
         reply[kReplyIsError] = true
         reply.send(err)
       }
-    } else if (reply[kReplySent] === false) {
+    } else if (reply.sent === false) {
       reply.log.error({ err: new FST_ERR_PROMISE_NOT_FULFILLED() }, "Promise may not be fulfilled with 'undefined' when statusCode is not 204")
     }
   }, function (err) {
-    if (reply[kReplySentOverwritten] === true || reply.sent === true) {
+    if (reply.sent === true) {
       reply.log.error({ err }, 'Promise errored, but reply.sent = true was set')
       return
     }
 
-    reply[kReplySent] = false
     reply[kReplyIsError] = true
     reply.send(err)
   })

test/internals/reply.test.js

@@ -1685,3 +1685,19 @@ test('reply.then', t => {
     response.destroy(_err)
   })
 })
+
+test('reply.sent should read from response.writableEnded if it is defined', t => {
+  t.plan(1)
+
+  const reply = new Reply({ writableEnded: true }, {}, {})
+
+  t.equal(reply.sent, true)
+})
+
+test('reply.sent should read from response.finished if response.writableEnded is not defined', t => {
+  t.plan(1)
+
+  const reply = new Reply({ finished: true }, {}, {})
+
+  t.equal(reply.sent, true)
+})

test/wrapThenable.test.js

@@ -4,6 +4,7 @@ const t = require('tap')
 const test = t.test
 const { kReplySentOverwritten } = require('../lib/symbols')
 const wrapThenable = require('../lib/wrapThenable')
+const Reply = require('../lib/reply')
 
 test('should resolve immediately when reply[kReplySentOverwritten] is true', t => {
   const reply = {}
@@ -15,7 +16,7 @@ test('should resolve immediately when reply[kReplySentOverwritten] is true', t =
 
 test('should reject immediately when reply[kReplySentOverwritten] is true', t => {
   t.plan(1)
-  const reply = { res: {} }
+  const reply = new Reply({}, {}, {})
   reply[kReplySentOverwritten] = true
   reply.log = {
     error: ({ err }) => {