From 26e8f8f3eb9904c1a3f71566a5c4331c8de59ee9 Mon Sep 17 00:00:00 2001
From: Raymond Feng <enjoyjava@gmail.com>
Date: Wed, 17 Oct 2018 09:43:07 -0700
Subject: [PATCH] fix headersSent check

This issue was discovered when an over-limit request body is
sent to LB4. Destroying the request socket causing the client
to fail instead of receiving a meaningful error.
---
 index.d.ts           |  3 ++-
 lib/handler.js       | 11 +++++++----
 test/handler.test.js | 23 +++++++++++++++++++++++
 3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/index.d.ts b/index.d.ts
index 646c652c..a66b4af6 100644
--- a/index.d.ts
+++ b/index.d.ts
@@ -27,13 +27,14 @@ declare namespace errorHandlerFactory {
    * @param req Incoming request
    * @param res Response
    * @param options Options for error handler settings
+   * @returns false if the response has been sent before the error
    */
   function writeErrorToResponse(
     err: Error,
     req: Express.Request,
     res: Express.Response,
     options?: ErrorWriterOptions
-  ): void;
+  ): boolean;
 
   /**
    * Error-handling middleware function. Includes server-side logging
diff --git a/lib/handler.js b/lib/handler.js
index 036a0ea2..936aa2b3 100644
--- a/lib/handler.js
+++ b/lib/handler.js
@@ -10,7 +10,6 @@ var SG = require('strong-globalize');
 SG.SetRootDir(path.resolve(__dirname, '..'));
 var buildResponseData = require('./data-builder');
 var debug = require('debug')('strong-error-handler');
-var format = require('util').format;
 var logToConsole = require('./logger');
 var negotiateContentProducer = require('./content-negotiation');
 
@@ -50,9 +49,12 @@ function writeErrorToResponse(err, req, res, options) {
 
   options = options || {};
 
-  if (res._header) {
-    debug('Response was already sent, closing the underlying connection');
-    return req.socket.destroy();
+  // See https://nodejs.org/api/http.html#http_response_headerssent
+  if (res.headersSent) {
+    debug('Response was already sent. Skipping error response.');
+    // We should not destroy the request socket as it causes Error: write EPIPE
+    // return req.socket.destroy();
+    return false;
   }
 
   // this will alter the err object, to handle when res.statusCode is an error
@@ -67,6 +69,7 @@ function writeErrorToResponse(err, req, res, options) {
 
   var sendResponse = negotiateContentProducer(req, warn, options);
   sendResponse(res, data);
+  return true;
 
   function warn(msg) {
     res.header('X-Warning', msg);
diff --git a/test/handler.test.js b/test/handler.test.js
index 06de6c28..c8da9848 100644
--- a/test/handler.test.js
+++ b/test/handler.test.js
@@ -38,6 +38,24 @@ describe('strong-error-handler', function() {
     request.get('/').expect(200, 'empty', done);
   });
 
+  it('handles response headers already sent without destroying the request',
+    function(done) {
+      givenErrorHandlerForError();
+      var handler = _requestHandler;
+      _requestHandler = function(req, res, next) {
+        res.end('empty');
+        process.nextTick(function() {
+          handler(req, res, next);
+        });
+      };
+      request.post('/').send(givenLargeRequest())
+        .expect(200, 'empty', function(err) {
+          // Skip EPIPE
+          if (err && err.code !== 'EPIPE') return done(err);
+          else return done();
+        });
+    });
+
   context('status code', function() {
     it('converts non-error "err.status" to 500', function(done) {
       givenErrorHandlerForError(new ErrorWithProps({status: 200}));
@@ -919,3 +937,8 @@ function getExpectedErrorData(err) {
   cloneAllProperties(data, err);
   return data;
 }
+
+function givenLargeRequest() {
+  const data = Buffer.alloc(2 * 1024 * 1024, 'A', 'utf-8');
+  return data.toString();
+}