Revert sent refactoring (#3108)

* Revert "fix #3099 (#3100)"

This reverts commit a848b0f.

* Revert "fixup! Simplify reply sent monitoring (#3072) (#3079)"

This reverts commit 9bf6a33.

* Revert "Simplify reply sent monitoring (#3072)"

This reverts commit 212fdb7.

lib/handleRequest.js

@@ -75,7 +75,9 @@ function handler (request, reply) {
 }
 
 function preValidationCallback (err, request, reply) {
-  if (reply.sent === true) return
+  if (reply.sent === true ||
+    reply.raw.writableEnded === true ||
+    reply.raw.writable === false) return
 
   if (err != null) {
     reply.send(err)
@@ -107,7 +109,9 @@ function preValidationCallback (err, request, reply) {
 }
 
 function preHandlerCallback (err, request, reply) {
-  if (reply.sent) return
+  if (reply.sent ||
+    reply.raw.writableEnded === true ||
+    reply.raw.writable === false) return
 
   if (err != null) {
     reply.send(err)

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,27 +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. The response.finished will be always true when the request
-      // method is HEAD and http2 is used, so we have to combine that with
-      // response.headersSent.
-      // 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.headersSent && this.raw.finished))) === true
+      return this[kReplySent]
     },
     set (value) {
       if (value !== true) {
         throw new FST_ERR_REP_SENT_VALUE()
       }
 
-      // We throw only if sent was overwritten from Fastify
-      if (this.sent && this[kReplySentOverwritten]) {
+      if (this[kReplySent]) {
         throw new FST_ERR_REP_ALREADY_SENT()
       }
 
       this[kReplySentOverwritten] = true
+      this[kReplySent] = true
     }
   },
   statusCode: {
@@ -115,7 +109,7 @@ Object.defineProperties(Reply.prototype, {
 })
 
 Reply.prototype.hijack = function () {
-  this[kReplySentOverwritten] = true
+  this[kReplySent] = true
   return this
 }
 
@@ -124,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
   }
@@ -383,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,
@@ -410,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.
@@ -439,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
@@ -508,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(
@@ -543,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
@@ -588,6 +589,7 @@ function handleError (reply, error, cb) {
     return
   }
 
+  reply[kReplySent] = true
   res.writeHead(res.statusCode, reply[kReplyHeaders])
   res.end(payload)
 }
@@ -649,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
@@ -661,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/handleRequest.test.js

@@ -1,6 +1,7 @@
 'use strict'
 
 const { test } = require('tap')
+const semver = require('semver')
 const handleRequest = require('../../lib/handleRequest')
 const internals = require('../../lib/handleRequest')[Symbol.for('internals')]
 const Request = require('../../lib/request')
@@ -107,8 +108,12 @@ test('handler function - preValidationCallback with finished response', t => {
   t.plan(0)
   const res = {}
   // Be sure to check only `writableEnded` where is available
-  res.writableEnded = true
-
+  if (semver.gte(process.versions.node, '12.9.0')) {
+    res.writableEnded = true
+  } else {
+    res.writable = false
+    res.finished = true
+  }
   res.end = () => {
     t.fail()
   }
@@ -133,7 +138,7 @@ test('handler function - preValidationCallback with finished response (< v12.9.0
   t.plan(0)
   const res = {}
   // Be sure to check only `writableEnded` where is available
-  res.headersSent = true
+  res.writable = false
   res.finished = true
 
   res.end = () => {

test/internals/reply.test.js

@@ -1329,7 +1329,6 @@ test('should throw error when attempting to set reply.sent more than once', t =>
     reply.sent = true
     try {
       reply.sent = true
-      t.fail('must throw')
     } catch (err) {
       t.equal(err.code, 'FST_ERR_REP_ALREADY_SENT')
       t.equal(err.message, 'Reply was already sent.')
@@ -1343,23 +1342,6 @@ test('should throw error when attempting to set reply.sent more than once', t =>
   })
 })
 
-test('should not throw error when attempting to set reply.sent if the underlining request was sent', t => {
-  t.plan(3)
-  const fastify = require('../..')()
-
-  fastify.get('/', function (req, reply) {
-    reply.raw.end()
-    t.doesNotThrow(() => {
-      reply.sent = true
-    })
-  })
-
-  fastify.inject('/', (err, res) => {
-    t.error(err)
-    t.pass()
-  })
-})
-
 test('reply.getResponseTime() should return 0 before the timer is initialised on the reply by setting up response listeners', t => {
   t.plan(1)
   const response = { statusCode: 200 }
@@ -1703,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.headersSent and response.finished if response.writableEnded is not defined', t => {
-  t.plan(1)
-
-  const reply = new Reply({ headersSent: true, 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 }) => {