Revert "Simplify reply sent monitoring (#3072)"

This reverts commit 212fdb7.

lib/reply.js

@@ -8,6 +8,7 @@ const {
   kSchemaResponse,
   kFourOhFourContext,
   kReplyErrorHandlerCalled,
+  kReplySent,
   kReplySentOverwritten,
   kReplyStartTime,
   kReplySerializer,
@@ -51,6 +52,7 @@ const warning = require('./warnings')
 
 function Reply (res, request, log) {
   this.raw = res
+  this[kReplySent] = false
   this[kReplySerializer] = null
   this[kReplyErrorHandlerCalled] = false
   this[kReplyIsError] = false
@@ -77,24 +79,19 @@ Object.defineProperties(Reply.prototype, {
   sent: {
     enumerable: true,
     get () {
-      // 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
+      return this[kReplySent]
     },
     set (value) {
       if (value !== true) {
         throw new FST_ERR_REP_SENT_VALUE()
       }
 
-      if (this.sent) {
+      if (this[kReplySent]) {
         throw new FST_ERR_REP_ALREADY_SENT()
       }
 
       this[kReplySentOverwritten] = true
+      this[kReplySent] = true
     }
   },
   statusCode: {
@@ -112,7 +109,7 @@ Object.defineProperties(Reply.prototype, {
 })
 
 Reply.prototype.hijack = function () {
-  this[kReplySentOverwritten] = true
+  this[kReplySent] = true
   return this
 }
 
@@ -121,7 +118,7 @@ Reply.prototype.send = function (payload) {
     throw new FST_ERR_SEND_INSIDE_ONERR()
   }
 
-  if (this.sent) {
+  if (this[kReplySent]) {
     this.log.warn({ err: new FST_ERR_REP_ALREADY_SENT() }, 'Reply already sent')
     return this
   }
@@ -380,6 +377,7 @@ function preserializeHookEnd (err, request, reply, payload) {
 }
 
 function onSendHook (reply, payload) {
+  reply[kReplySent] = true
   if (reply.context.onSend !== null) {
     onSendHookRunner(
       reply.context.onSend,
@@ -407,6 +405,8 @@ 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.
@@ -436,6 +436,8 @@ function onSendEnd (reply, payload) {
     reply[kReplyHeaders]['content-length'] = '' + Buffer.byteLength(payload)
   }
 
+  reply[kReplySent] = true
+
   res.writeHead(statusCode, reply[kReplyHeaders])
 
   // avoid ArgumentsAdaptorTrampoline from V8
@@ -505,6 +507,7 @@ 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(
@@ -540,6 +543,7 @@ 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,6 +589,7 @@ function handleError (reply, error, cb) {
     return
   }
 
+  reply[kReplySent] = true
   res.writeHead(res.statusCode, reply[kReplyHeaders])
   res.end(payload)
 }
@@ -646,6 +651,7 @@ function buildReply (R) {
     this.raw = res
     this[kReplyIsError] = false
     this[kReplyErrorHandlerCalled] = false
+    this[kReplySent] = false
     this[kReplySentOverwritten] = false
     this[kReplySerializer] = null
     this.request = request
@@ -658,6 +664,7 @@ function buildReply (R) {
 }
 
 function notFound (reply) {
+  reply[kReplySent] = false
   reply[kReplyIsError] = false
 
   if (reply.context[kFourOhFourContext] === null) {

lib/symbols.js

@@ -30,6 +30,7 @@ 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,6 +2,7 @@
 
 const {
   kReplyIsError,
+  kReplySent,
   kReplySentOverwritten
 } = require('./symbols')
 
@@ -15,24 +16,26 @@ function wrapThenable (thenable, reply) {
 
     // this is for async functions that
     // are using reply.send directly
-    if (payload !== undefined || (reply.raw.statusCode === 204 && reply.sent === false)) {
+    if (payload !== undefined || (reply.raw.statusCode === 204 && reply[kReplySent] === 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.sent === false) {
+    } else if (reply[kReplySent] === 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.sent === true) {
+    if (reply[kReplySentOverwritten] === true || 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,19 +1685,3 @@ 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,7 +4,6 @@ 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 = {}
@@ -16,7 +15,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 = new Reply({}, {}, {})
+  const reply = { res: {} }
   reply[kReplySentOverwritten] = true
   reply.log = {
     error: ({ err }) => {