fix!: send correct response code on exception

Author: matthewrobertson

src/invoker.ts

@@ -21,7 +21,6 @@
 //     functions with HTTP trigger).
 import * as express from 'express';
 import * as http from 'http';
-import {FUNCTION_STATUS_HEADER_FIELD} from './types';
 import {sendCrashResponse} from './logger';
 
 /**
@@ -47,10 +46,7 @@ export function sendResponse(
   res: express.Response
 ) {
   if (err) {
-    res.set(FUNCTION_STATUS_HEADER_FIELD, 'error');
-    // Sending error message back is fine for Pub/Sub-based functions as they do
-    // not reach the caller anyway.
-    res.send(err.message);
+    sendCrashResponse({err, res, statusHeader: 'error'});
     return;
   }
   if (typeof result === 'undefined' || result === null) {

src/logger.ts

@@ -27,12 +27,14 @@ export function sendCrashResponse({
   res,
   callback,
   silent = false,
+  statusHeader = 'crash',
 }: {
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
   err: Error | any;
   res: express.Response | null;
   callback?: Function;
   silent?: boolean;
+  statusHeader?: string;
 }) {
   if (!silent) {
     console.error(err.stack || err);
@@ -43,8 +45,14 @@ export function sendCrashResponse({
   // right before sending the response, to make sure that no concurrent
   // execution sends the response between the check and 'send' call below.
   if (res && !res.headersSent) {
-    res.set(FUNCTION_STATUS_HEADER_FIELD, 'crash');
-    res.send((err.message || err) + '');
+    res.set(FUNCTION_STATUS_HEADER_FIELD, statusHeader);
+    
+    if (process.env.NODE_ENV !== 'production') {
+      res.status(500);
+      res.send((err.message || err) + '');
+    } else {
+      res.sendStatus(500);
+    }
   }
   if (callback) {
     callback();

test/conformance/function.js

@@ -5,7 +5,7 @@ const fileName = 'function_output.json';
 
 functions.http('writeHttpDeclarative', (req, res) => {
   writeJson(req.body);
-  res.end(200);
+  res.sendStatus(200);
 });
 
 functions.cloudEvent('writeCloudEventDeclarative', cloudEvent => {
@@ -15,7 +15,7 @@ functions.cloudEvent('writeCloudEventDeclarative', cloudEvent => {
 
 function writeHttp(req, res) {
   writeJson(req.body);
-  res.end(200);
+  res.sendStatus(200);
 }
 
 function writeCloudEvent(cloudEvent) {

test/integration/cloud_event.ts

@@ -50,11 +50,13 @@ describe('CloudEvent Function', () => {
     clock = sinon.useFakeTimers();
     // Prevent log spew from the PubSub emulator request.
     sinon.stub(console, 'warn');
+    sinon.stub(console, 'error');
   });
 
   afterEach(() => {
     clock.restore();
     (console.warn as sinon.SinonSpy).restore();
+    (console.error as sinon.SinonSpy).restore();
   });
 
   const testData = [
@@ -302,4 +304,21 @@ describe('CloudEvent Function', () => {
       })
       .expect(204);
   });
+
+  it('returns a 500 if the function throws an exception', async () => {
+    functions.cloudEvent('testTypedCloudEvent', () => {
+      throw 'I crashed';
+    });
+
+    // invoke the function with a CloudEvent with a string payload
+    const server = getTestServer('testTypedCloudEvent');
+    await supertest(server)
+      .post('/')
+      .send(TEST_CLOUD_EVENT)
+      .expect(res => {
+        assert.deepStrictEqual(res.headers['x-google-status'], 'error');
+        assert.deepStrictEqual(res.body, {});
+      })
+      .expect(500);
+  });
 });

test/integration/http.ts

@@ -13,6 +13,7 @@
 // limitations under the License.
 
 import * as assert from 'assert';
+import * as sinon from 'sinon';
 import * as supertest from 'supertest';
 
 import * as functions from '../../src/index';
@@ -24,14 +25,25 @@ describe('HTTP Function', () => {
   before(() => {
     functions.http('testHttpFunction', (req, res) => {
       ++callCount;
+      if (req.query.crash) {
+        throw 'I crashed';
+      }
       res.send({
         result: req.body.text,
         query: req.query.param,
       });
     });
   });
 
-  beforeEach(() => (callCount = 0));
+  beforeEach(() => {
+    callCount = 0;
+    // Prevent log spew from the PubSub emulator request.
+    sinon.stub(console, 'error');
+  });
+
+  afterEach(() => {
+    (console.error as sinon.SinonSpy).restore();
+  });
 
   const testData = [
     {
@@ -58,6 +70,14 @@ describe('HTTP Function', () => {
       expectedStatus: 200,
       expectedCallCount: 1,
     },
+    {
+      name: 'GET throws exception',
+      httpVerb: 'GET',
+      path: '/foo?crash=true',
+      expectedBody: {},
+      expectedStatus: 500,
+      expectedCallCount: 1,
+    },
     {
       name: 'GET favicon.ico',
       httpVerb: 'GET',

test/integration/legacy_event.ts

@@ -14,6 +14,7 @@
 
 import * as assert from 'assert';
 import * as functions from '../../src/functions';
+import * as sinon from 'sinon';
 import {getServer} from '../../src/server';
 import * as supertest from 'supertest';
 
@@ -31,6 +32,15 @@ const TEST_CLOUD_EVENT = {
 };
 
 describe('Event Function', () => {
+  beforeEach(() => {
+    // Prevent log spew from the PubSub emulator request.
+    sinon.stub(console, 'error');
+  });
+
+  afterEach(() => {
+    (console.error as sinon.SinonSpy).restore();
+  });
+
   const testData = [
     {
       name: 'GCF event',
@@ -188,4 +198,25 @@ describe('Event Function', () => {
       assert.deepStrictEqual(receivedContext, test.expectedContext);
     });
   });
+
+  it('returns a 500 if the function throws an exception', async () => {
+    const server = getServer(() => {
+      throw 'I crashed';
+    }, 'event');
+    await supertest(server)
+      .post('/')
+      .send({
+        eventId: 'testEventId',
+        timestamp: 'testTimestamp',
+        eventType: 'testEventType',
+        resource: 'testResource',
+        data: {some: 'payload'},
+      })
+      .set({'Content-Type': 'application/json'})
+      .expect(res => {
+        assert.deepStrictEqual(res.headers['x-google-status'], 'error');
+        assert.deepStrictEqual(res.body, {});
+      })
+      .expect(500);
+  });
 });