Skip to content

Commit

Permalink
Correctly handle exception from 'done'.
Browse files Browse the repository at this point in the history
  • Loading branch information
Jason Walton committed May 2, 2017
1 parent ad99eca commit b152825
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 4 deletions.
13 changes: 11 additions & 2 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,9 @@
'return function(' + toList(args, 'done') + ') {\n' +
' if(done) {\n' +
' promiseFn.call(' + toList(params) + ').then(\n' +
' function(result) {done(null, result);},\n' +
// Call `done()` inside `setTimeout()`, so that if `done` throws an error, it will
// be turned into an uncaught exception, instead of being swallowed by the Promise.
' function(result) {setTimeout(function() {done(null, result);}, 0);},\n' +
' function(err) {done(err);}\n' +
' );\n' +
' return null;\n' +
Expand Down Expand Up @@ -156,7 +158,14 @@
}

if(done) {
answer.then(function(result) {done(null, result);}, done);
answer.then(
function(result) {
// Call `done()` inside `setTimeout()`, so that if `done` throws an error, it will
// be turned into an uncaught exception, instead of being swallowed by the Promise.
setTimeout(function() {done(null, result);}, 0);
},
done
);
answer = null;
}

Expand Down
23 changes: 21 additions & 2 deletions test/addCallbackTest.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
'use strict';

const chai = require('chai');
const pb = require('..');

chai.use(require('chai-as-promised'));
const {expect} = chai;

const {expectUncaughtException} = require('./testUtils');
const pb = require('..');

describe('addCallback', () => {
[
{fn: Promise.resolve(7), comment: 'promise'},
Expand All @@ -27,8 +29,12 @@ describe('addCallback', () => {
});
});

const rejected = Promise.reject(new Error('boom'));
// This is here to stop Node from raising a "Potentially Unhandled Rejection Error".
rejected.catch(() => null);

[
{fn: Promise.reject(new Error('boom')), comment: 'promise'},
{fn: rejected, comment: 'promise'},
{fn: () => Promise.reject(new Error('boom')), comment: 'fn'},
{fn: () => {throw new Error('boom');}, comment: 'throw'}
].forEach(({fn, comment}) => {
Expand Down Expand Up @@ -57,4 +63,17 @@ describe('addCallback', () => {
() => pb.addCallback(null, {})
).to.throw('addCallback() don\'t know what to do with object');
});

it('should deal correctly with exception from `done`', () => {
const fn = (done=null) => pb.addCallback(done, Promise.resolve('hello'));
const done = () => {throw new Error('boom')};

// We're calling into `fn`, but `done` is throwing an exception after `fn` is complete. In
// an all-callback based world, this would cause an uncaught exception. There's no better way to
// handle this, so make sure we get an uncaught exception here.
return expectUncaughtException(
() => fn(done)
);

});
});
13 changes: 13 additions & 0 deletions test/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const chai = require('chai');
chai.use(require('chai-as-promised'));
const {expect} = chai;

const {expectUncaughtException} = require('./testUtils');
const promiseBreaker = require('../index');

const makeTestCases = function(testFn, fns) {
Expand Down Expand Up @@ -253,4 +254,16 @@ describe("Use custom promise", () => {
expect(result).to.equal(3);
});
});

it('should deal correctly with exception from `done`', () => {
const fn = promiseBreaker.break(() => Promise.resolve('hello'));
const done = () => {throw new Error('boom')};

// We're calling into `fn`, but `done` is throwing an exception after `fn` is complete. In
// an all-callback based world, this would cause an uncaught exception. There's no better way to
// handle this, so make sure we get an uncaught exception here.
return expectUncaughtException(
() => fn(done)
);
});
});
13 changes: 13 additions & 0 deletions test/testUtils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
exports.expectUncaughtException = function(fn) {
// Remove mocha's uncaughtException handler.
const originalListeners = process.listeners('uncaughtException');
originalListeners.forEach(listener => process.removeListener('uncaughtException', listener));

return new Promise(resolve => {
process.once('uncaughtException', resolve);
fn();
})
.then(() => {
originalListeners.forEach(listener => process.on('uncaughtException', listener))
});
}

0 comments on commit b152825

Please sign in to comment.