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

Avoid async operations while protect is running #1619

Closed
wants to merge 3 commits into from
Closed
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
11 changes: 4 additions & 7 deletions lib/protect.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,6 @@ exports = module.exports = internals.Protect = function (parent) {
this._parent = parent;
this._error = null;


this._baseDomain = Domain.createDomain(); // Envelope domain to force exit all nested inner domains

this.domain = Domain.createDomain();
this.domain.on('error', function (err) {

Expand All @@ -42,7 +39,6 @@ internals.Protect.prototype.run = function (next, setup) { // setup: fu

var finish = function (/* arguments */) {

self._baseDomain.exit(); // Forces exit from this.domain along with any nested domains
self._error = null;

return next.apply(null, arguments);
Expand All @@ -52,14 +48,15 @@ internals.Protect.prototype.run = function (next, setup) { // setup: fu

this._error = function (err) {

// Exit out of the implict request domain so we can isolate user and framework code.
self.domain.exit();

return finish(Boom.badImplementation('Uncaught error', err));
};

var protect = function (run) {

self._baseDomain.enter();
self.domain.enter();
run();
self.domain.run(run);
};

setup(protect, finish);
Expand Down
43 changes: 43 additions & 0 deletions test/protect.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ var Events = require('events');
var Lab = require('lab');
var Hapi = require('..');
var Hoek = require('hoek');
var Nipple = require('nipple');


// Declare internals
Expand Down Expand Up @@ -43,6 +44,48 @@ describe('Protect', function () {
});
});


it('does not propagate the domain outside of request lifetime', function (done) {

// The server must be run without the test domain to demonstrate the issue.
// In isolation this is unnecessary but multiple tests somehow leave the domain
// state in a somehow broken state.
while (process.domain) {
process.domain.exit();
}

var server = new Hapi.Server(0);
var executed = false;

var handler = function (request, reply) {

expect(process.domain).to.equal(request.domain);
expect(request.raw.req.socket.domain).to.equal(null);

if (!executed) {
executed = true;
Nipple.request('GET', 'http://localhost:' + server.info.port + '/', {}, function (err, res) {
expect(res.statusCode).to.equal(200);
reply('ok');
});
} else {
reply('ok');
}
};

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

require('domain').log = true;
Nipple.request('GET', 'http://localhost:' + server.info.port + '/', {}, function (err, res) {

server.stop();
expect(res.statusCode).to.equal(200);
done();
});
});
});

it('catches error when handler throws twice after reply() is called', function (done) {

var server = new Hapi.Server({ debug: false });
Expand Down