Skip to content

Commit

Permalink
Improve checks for Error in onerror handlers
Browse files Browse the repository at this point in the history
  • Loading branch information
julienw committed Jun 2, 2020
1 parent 9ee6584 commit 244a1b4
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 4 deletions.
8 changes: 7 additions & 1 deletion lib/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,13 @@ module.exports = class Application extends Emitter {
*/

onerror(err) {
if (!(err instanceof Error)) throw new TypeError(util.format('non-error thrown: %j', err));
// When dealing with cross-globals a normal `instanceof` check doesn't work properly.
// See https://github.com/koajs/koa/issues/1466
// We can probably remove it once jest fixes https://github.com/facebook/jest/issues/2549.
const isNativeError =
Object.prototype.toString.call(err) === '[object Error]' ||
err instanceof Error;
if (!isNativeError) throw new TypeError(util.format('non-error thrown: %j', err));

if (404 === err.status || err.expose) return;
if (this.silent) return;
Expand Down
8 changes: 7 additions & 1 deletion lib/context.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,13 @@ const proto = module.exports = {
// to node-style callbacks.
if (null == err) return;

if (!(err instanceof Error)) err = new Error(util.format('non-error thrown: %j', err));
// When dealing with cross-globals a normal `instanceof` check doesn't work properly.
// See https://github.com/koajs/koa/issues/1466
// We can probably remove it once jest fixes https://github.com/facebook/jest/issues/2549.
const isNativeError =
Object.prototype.toString.call(err) === '[object Error]' ||
err instanceof Error;
if (!isNativeError) err = new Error(util.format('non-error thrown: %j', err));

let headerSent = false;
if (this.headerSent || !this.writable) {
Expand Down
12 changes: 12 additions & 0 deletions test/application/onerror.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,18 @@ describe('app.onerror(err)', () => {
}, TypeError, 'non-error thrown: foo');
});

it('should accept errors coming from other scopes', () => {
const ExternError = require('vm').runInNewContext('Error');

const app = new Koa();
const error = Object.assign(new ExternError('boom'), {
status: 418,
expose: true
});

assert.doesNotThrow(() => app.onerror(error));
});

it('should do nothing if status is 404', () => {
const app = new Koa();
const err = new Error();
Expand Down
37 changes: 35 additions & 2 deletions test/context/onerror.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@

'use strict';

const assert = require('assert');
Expand Down Expand Up @@ -205,6 +204,40 @@ describe('ctx.onerror(err)', () => {
});
});

describe('when error from another scope thrown', () => {
it('should handle it like a normal error', async() => {
const ExternError = require('vm').runInNewContext('Error');

const app = new Koa();
const error = Object.assign(new ExternError('boom'), {
status: 418,
expose: true
});
app.use((ctx, next) => {
throw error;
});

const server = app.listen();

const gotRightErrorPromise = new Promise((resolve, reject) => {
app.on('error', receivedError => {
try {
assert.strictEqual(receivedError, error);
resolve();
} catch (e) {
reject(e);
}
});
});

await request(server)
.get('/')
.expect(418);

await gotRightErrorPromise;
});
});

describe('when non-error thrown', () => {
it('should response non-error thrown message', () => {
const app = new Koa();
Expand Down Expand Up @@ -248,7 +281,7 @@ describe('ctx.onerror(err)', () => {
});

app.use(async ctx => {
throw {key: 'value'}; // eslint-disable-line no-throw-literal
throw { key: 'value' }; // eslint-disable-line no-throw-literal
});

request(app.callback())
Expand Down

0 comments on commit 244a1b4

Please sign in to comment.