Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide cover for unhandled rejections #3486

Merged
merged 3 commits into from
Jun 5, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions API.md
Original file line number Diff line number Diff line change
Expand Up @@ -2871,6 +2871,30 @@ const handler = function (request, reply) {
};
```

If the handler returns a Promise then Hapi will register a `catch` handler on the promise object to catch unhandled promise rejections. The handler will `reply` with the rejected value, wrapped in a [`Boom`](https://github.com/hapijs/boom) error:

```js
const handler = function (request, reply) {

const badPromise = () => {

new Promise((resolve, reject) => {

// Hapi catches this...
throw new Error();

// ...and this...
return reject(new Error());
}
}

// ...if you don't provide a 'catch'. The rejection will be wrapped in a Boom error.
return badPromise().then(reply);
}
```

This provides a safety net for unhandled promise rejections.

### Route prerequisites

It is often necessary to perform prerequisite actions before the handler is called (e.g. load
Expand Down
20 changes: 19 additions & 1 deletion lib/handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
// Load modules

const Hoek = require('hoek');
const Boom = require('boom');
const Items = require('items');
const Methods = require('./methods');
const Promises = require('./promises');
const Response = require('./response');


Expand Down Expand Up @@ -96,7 +98,23 @@ internals.handler = function (request, callback) {

// Execute handler

request.route.settings.handler.call(bind, request, reply);
const handlerResult = request.route.settings.handler.call(bind, request, reply);

// Give hapi a fighting chance to deal with uncaught rejections.
if (Promises.isThennable(handlerResult)) {

handlerResult.then(null, (error) => {

// Unhandled rejections are always Boom-ified, just like uncaught exceptions.
if (error instanceof Error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it common practice to reject a promise with a non-Error object?!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i sure hope not..

Copy link
Member

@devinivy devinivy May 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You certainly aren't supposed to, but out in the wild it happens. As to whether hapi should deal with the rejection of a non-error, I can't say. I would probably treat it the same way hapi deals with a non-error thrown in a handler.

Copy link

@atrauzzi atrauzzi May 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, can happen. Have seen strings thrown in the past.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hueniverse it is definitely non-conventional to reject a non-error object. If done deliberately it would definitely be an anti-pattern. However, there are cases where it will slip in by mistake, here's the most common scenario:

function doSomethingAsynchronous(input) {
  return new Promise((resolve, reject) => {
    callbackToLibrary(input, (err, result) => {
      if (err) return reject(err);  // uh-oh....
      return resolve(result);
    });
  });
}

In this case, if the library you are using does something goofy like callback with a non-error object, the promise will reject with the non-error object.

I think that with the handling of thrown exceptions in domains we have the same behaviour (because Boom.wrap covers whatever is thrown, ensuring that even if what is thrown is not an error, it gets wrapped in one).

return reply(Boom.wrap(error));
}

const err = new Error('Rejected promise');
err.data = error;
return reply(Boom.wrap(err));
});
}
};


Expand Down
6 changes: 6 additions & 0 deletions lib/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,9 @@ exports.wrap = function (bind, method, args) {
method.apply(bind, args ? args.concat(callback) : [callback]);
});
};

exports.isThennable = function (candidate) {

return candidate && (typeof candidate.then === 'function') ? true : false;

};
101 changes: 101 additions & 0 deletions test/handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,107 @@ describe('handler', () => {
done();
});
});

it('catches asynchronous exceptions in promise constructors via the standard \'protect\' mechanism', (done) => {

const server = new Hapi.Server({ debug: false });
server.connection();

const asyncOperation = function () {

return new Promise((resolve, reject) => {

setTimeout(() => {

throw new Error('This should be caught...');
}, 100);
});
};

const handler = function (request, reply) {

return asyncOperation()
.then(reply);
};

server.route({ method: 'GET', path: '/', handler });

server.inject('/', (res) => {

expect(res.statusCode).to.equal(500);
expect(res.result.error).to.equal('Internal Server Error');
done();
});
});

it('catches synchronous exceptions which lead to unhandled promise rejections when then promise is returned', (done) => {

const server = new Hapi.Server();
server.connection();

const asyncOperation = function () {

return new Promise((resolve, reject) => {

throw new Error('This should be rejected...');
Copy link
Contributor

@AJamesPhillips AJamesPhillips May 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add a test for your setTimeout(() => throw new Error(), 1000); too from above.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/catch#Gotchas_when_throwing_errors

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call! Adding now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the test, it's there now :)

});
};

const handler = function (request, reply) {

return asyncOperation()
.then(reply);
};

server.route({ method: 'GET', path: '/', handler });

server.inject('/', (res) => {

expect(res.statusCode).to.equal(500);
expect(res.result.error).to.equal('Internal Server Error');
done();
});
});

it('catches unhandled promise rejections when a promise is returned', (done) => {

const server = new Hapi.Server();
server.connection();

const handler = function (request, reply) {

return Promise.reject(new Error('This should be caught.'));
};

server.route({ method: 'GET', path: '/', handler });

server.inject('/', (res) => {

expect(res.statusCode).to.equal(500);
expect(res.result.error).to.equal('Internal Server Error');
done();
});
});

it('wraps unhandled promise rejections in an error if needed', (done) => {

const server = new Hapi.Server();
server.connection();

const handler = function (request, reply) {

return Promise.reject('this should be wrapped in an error');
};

server.route({ method: 'GET', path: '/', handler });

server.inject('/', (res) => {

expect(res.statusCode).to.equal(500);
expect(res.result.error).to.equal('Internal Server Error');
done();
});
});
});

describe('register()', () => {
Expand Down
43 changes: 43 additions & 0 deletions test/promises.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
'use strict';

// Load modules

const Code = require('code');
const Lab = require('lab');
const Promises = require('../lib/promises');

// Declare internals

const internals = {};


// Test shortcuts

const lab = exports.lab = Lab.script();
const describe = lab.describe;
const it = lab.it;
const expect = Code.expect;


describe('promise', () => {

it('recognises a promise as thennable', (done) => {

const promise = new Promise(() => {});
expect(Promises.isThennable(promise)).to.equal(true);

done();

});

it('recognises invalid input as not thennable', (done) => {

expect(Promises.isThennable(undefined)).to.equal(false);
expect(Promises.isThennable(null)).to.equal(false);
expect(Promises.isThennable({ })).to.equal(false);
expect(Promises.isThennable({ then: true })).to.equal(false);

done();

});
});