From 907caa1e93c1b66df0b76389e1fc7b3cfdc4d3e4 Mon Sep 17 00:00:00 2001
From: Ruben Verborgh <ruben@verborgh.org>
Date: Tue, 8 Dec 2020 19:50:28 +0000
Subject: [PATCH] fix: Do not write error if response already started.

---
 src/server/ExpressHttpServerFactory.ts        |  6 +-
 .../server/ExpressHttpServerFactory.test.ts   | 76 +++++++------------
 2 files changed, 33 insertions(+), 49 deletions(-)

diff --git a/src/server/ExpressHttpServerFactory.ts b/src/server/ExpressHttpServerFactory.ts
index db5249d85..22f72c8b9 100644
--- a/src/server/ExpressHttpServerFactory.ts
+++ b/src/server/ExpressHttpServerFactory.ts
@@ -28,7 +28,11 @@ export class ExpressHttpServerFactory implements HttpServerFactory {
       } catch (error: unknown) {
         const errMsg = error instanceof Error ? `${error.name}: ${error.message}\n${error.stack}` : 'Unknown error.';
         this.logger.error(errMsg);
-        response.status(500).contentType('text/plain').send(errMsg);
+        if (response.headersSent) {
+          response.end();
+        } else {
+          response.status(500).contentType('text/plain').send(errMsg);
+        }
       } finally {
         done();
       }
diff --git a/test/unit/server/ExpressHttpServerFactory.test.ts b/test/unit/server/ExpressHttpServerFactory.test.ts
index 335f41d11..b78b82cc0 100644
--- a/test/unit/server/ExpressHttpServerFactory.test.ts
+++ b/test/unit/server/ExpressHttpServerFactory.test.ts
@@ -1,59 +1,33 @@
 import type { Server } from 'http';
 import request from 'supertest';
 import { ExpressHttpServerFactory } from '../../../src/server/ExpressHttpServerFactory';
-import { HttpHandler } from '../../../src/server/HttpHandler';
-import type { HttpRequest } from '../../../src/server/HttpRequest';
+import type { HttpHandler } from '../../../src/server/HttpHandler';
 import type { HttpResponse } from '../../../src/server/HttpResponse';
-import SpyInstance = jest.SpyInstance;
 
-const handle = async(input: { request: HttpRequest; response: HttpResponse }): Promise<void> => {
-  input.response.writeHead(200);
-  input.response.end();
-};
-
-class SimpleHttpHandler extends HttpHandler {
-  public async handle(input: { request: HttpRequest; response: HttpResponse }): Promise<void> {
-    return handle(input);
-  }
-}
+const handler: jest.Mocked<HttpHandler> = {
+  handleSafe: jest.fn(async(input: { response: HttpResponse }): Promise<void> => {
+    input.response.writeHead(200);
+    input.response.end();
+  }),
+} as any;
 
 describe('ExpressHttpServerFactory', (): void => {
   let server: Server;
-  let canHandleJest: jest.Mock<Promise<void>, []>;
-  let handleJest: jest.Mock<Promise<void>, [any]>;
-  let handler: SimpleHttpHandler;
-  let mock: SpyInstance;
 
   beforeAll(async(): Promise<void> => {
-    // Prevent test from writing to stderr
-    mock = jest.spyOn(process.stderr, 'write').mockImplementation((): boolean => true);
-  });
-
-  beforeEach(async(): Promise<void> => {
-    handler = new SimpleHttpHandler();
-    canHandleJest = jest.fn(async(): Promise<void> => undefined);
-    handleJest = jest.fn(async(input): Promise<void> => handle(input));
-
-    handler.canHandle = canHandleJest;
-    handler.handle = handleJest;
-
     const factory = new ExpressHttpServerFactory(handler);
     server = factory.startServer(5555);
   });
 
-  afterEach(async(): Promise<void> => {
-    server.close();
-  });
-
   afterAll(async(): Promise<void> => {
-    mock.mockReset();
+    server.close();
   });
 
   it('sends incoming requests to the handler.', async(): Promise<void> => {
     await request(server).get('/').set('Host', 'test.com').expect(200);
-    expect(canHandleJest).toHaveBeenCalledTimes(1);
-    expect(handleJest).toHaveBeenCalledTimes(1);
-    expect(handleJest).toHaveBeenLastCalledWith({
+
+    expect(handler.handleSafe).toHaveBeenCalledTimes(1);
+    expect(handler.handleSafe).toHaveBeenLastCalledWith({
       request: expect.objectContaining({
         headers: expect.objectContaining({ host: 'test.com' }),
       }),
@@ -62,25 +36,31 @@ describe('ExpressHttpServerFactory', (): void => {
   });
 
   it('returns a 404 when the handler does not do anything.', async(): Promise<void> => {
-    handler.handle = async(input): Promise<void> => {
-      expect(input).toBeDefined();
-    };
-    await request(server).get('/').expect(404);
+    handler.handleSafe.mockResolvedValueOnce(undefined);
+
+    await expect(request(server).get('/').expect(404)).resolves.toBeDefined();
   });
 
-  it('catches errors thrown by its handler.', async(): Promise<void> => {
-    handler.handle = async(): Promise<void> => {
-      throw new Error('dummyError');
-    };
+  it('writes an error to the HTTP response.', async(): Promise<void> => {
+    handler.handleSafe.mockRejectedValueOnce(new Error('dummyError'));
 
     const res = await request(server).get('/').expect(500);
+    expect(res.headers['content-type']).toBe('text/plain; charset=utf-8');
     expect(res.text).toContain('dummyError');
   });
 
+  it('does not write an error if the response had been started.', async(): Promise<void> => {
+    handler.handleSafe.mockImplementationOnce(async(input: { response: HttpResponse }): Promise<void> => {
+      input.response.write('content');
+      throw new Error('dummyError');
+    });
+
+    const res = await request(server).get('/');
+    expect(res.text).not.toContain('dummyError');
+  });
+
   it('throws unknown errors if its handler throw non-Error objects.', async(): Promise<void> => {
-    handler.handle = async(): Promise<void> => {
-      throw 'apple';
-    };
+    handler.handleSafe.mockRejectedValueOnce('apple');
 
     const res = await request(server).get('/').expect(500);
     expect(res.text).toContain('Unknown error.');