fix!: send correct response code on exception (OpenFunction#427)

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);
+ });
 });