Skip to content

Commit 2187fa3

Browse files
author
Eran Hammer
committed
Fix request-error emit multiple times. Closes hapijs#2326 
1 parent 620e443 commit 2187fa3

File tree

5 files changed

+170
-34
lines changed

5 files changed

+170
-34
lines changed

‎lib/request.js

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -385,6 +385,15 @@ internals.Request.prototype._reply = function (exit) {
385385
internals.Request.prototype._finalize = function () {
386386

387387
this.info.responded = Date.now();
388+
389+
if (this.response &&
390+
this.response.statusCode === 500 &&
391+
this.response._error) {
392+
393+
this.connection.emit('request-error', this, this.response._error);
394+
this._log(this.response._error.isDeveloperError ? ['internal', 'implementation', 'error'] : ['internal', 'error'], this.response._error);
395+
}
396+
388397
this.connection.emit('response', this);
389398

390399
this._isFinalized = true;
@@ -430,13 +439,6 @@ internals.Request.prototype._setResponse = function (response) {
430439
}
431440

432441
this.response = response;
433-
434-
if (response.isBoom &&
435-
response.output.statusCode === 500) {
436-
437-
this.connection.emit('request-error', this, response);
438-
this._log(response.isDeveloperError ? ['internal', 'implementation', 'error'] : ['internal', 'error'], response);
439-
}
440442
};
441443

442444

‎lib/response.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ exports = module.exports = internals.Response = function (source, request, optio
4040
this._payload = null; // Readable stream
4141
this._takeover = false;
4242
this._contentEncoding = null; // Set during transmit
43+
this._error = null; // The boom object when created from an error
4344

4445
this._processors = {
4546
marshal: options.marshal,

‎lib/transmit.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,7 @@ internals.fail = function (request, boom, callback) {
146146

147147
var error = boom.output;
148148
var response = new Response(error.payload, request);
149+
response._error = boom;
149150
response.code(error.statusCode);
150151
response.headers = error.headers;
151152
request.response = response; // Not using request._setResponse() to avoid double log

‎test/request.js

Lines changed: 128 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -409,6 +409,106 @@ describe('Request', function () {
409409
done();
410410
});
411411
});
412+
413+
it('emits request-error once', function (done) {
414+
415+
var server = new Hapi.Server({ debug: false });
416+
server.connection();
417+
418+
var errs = 0;
419+
var req = null;
420+
server.on('request-error', function (request, err) {
421+
422+
errs++;
423+
expect(err).to.exist();
424+
expect(err.message).to.equal('boom2');
425+
req = request;
426+
});
427+
428+
server.ext('onPreResponse', function (request, reply) {
429+
430+
return reply(new Error('boom2'));
431+
});
432+
433+
server.route({ method: 'GET', path: '/', handler: function (request, reply) { return reply(new Error('boom1')); } });
434+
435+
server.inject('/', function (res) {
436+
437+
expect(res.statusCode).to.equal(500);
438+
expect(res.result).to.exist();
439+
expect(res.result.message).to.equal('An internal server error occurred');
440+
});
441+
442+
server.once('response', function () {
443+
444+
expect(errs).to.equal(1);
445+
expect(req.getLog('error')[1].tags).to.deep.equal(['internal', 'error']);
446+
done();
447+
});
448+
});
449+
450+
it('emits request-error on implementation error', function (done) {
451+
452+
var server = new Hapi.Server({ debug: false });
453+
server.connection();
454+
455+
var errs = 0;
456+
var req = null;
457+
server.on('request-error', function (request, err) {
458+
459+
errs++;
460+
expect(err).to.exist();
461+
expect(err.message).to.equal('Uncaught error: boom');
462+
req = request;
463+
});
464+
465+
server.route({ method: 'GET', path: '/', handler: function (request, reply) { throw new Error('boom'); } });
466+
467+
server.inject('/', function (res) {
468+
469+
expect(res.statusCode).to.equal(500);
470+
expect(res.result).to.exist();
471+
expect(res.result.message).to.equal('An internal server error occurred');
472+
});
473+
474+
server.once('response', function () {
475+
476+
expect(errs).to.equal(1);
477+
expect(req.getLog('error')[0].tags).to.deep.equal(['internal', 'implementation', 'error']);
478+
done();
479+
});
480+
});
481+
482+
it('does not emit request-error when error is replaced with valid response', function (done) {
483+
484+
var server = new Hapi.Server({ debug: false });
485+
server.connection();
486+
487+
var errs = 0;
488+
server.on('request-error', function (request, err) {
489+
490+
errs++;
491+
});
492+
493+
server.ext('onPreResponse', function (request, reply) {
494+
495+
return reply('ok');
496+
});
497+
498+
server.route({ method: 'GET', path: '/', handler: function (request, reply) { return reply(new Error('boom1')); } });
499+
500+
server.inject('/', function (res) {
501+
502+
expect(res.statusCode).to.equal(200);
503+
expect(res.result).to.equal('ok');
504+
});
505+
506+
server.once('response', function () {
507+
508+
expect(errs).to.equal(0);
509+
done();
510+
});
511+
});
412512
});
413513

414514
describe('tail()', function () {
@@ -732,6 +832,34 @@ describe('Request', function () {
732832
});
733833
});
734834

835+
it('outputs log to debug console with error data', function (done) {
836+
837+
var handler = function (request, reply) {
838+
839+
request.log(['implementation'], new Error('boom'));
840+
return reply();
841+
};
842+
843+
var server = new Hapi.Server();
844+
server.connection();
845+
server.route({ method: 'GET', path: '/', handler: handler });
846+
847+
var orig = console.error;
848+
console.error = function () {
849+
850+
expect(arguments[0]).to.equal('Debug:');
851+
expect(arguments[1]).to.equal('implementation');
852+
expect(arguments[2]).to.contain('Error: boom');
853+
console.error = orig;
854+
done();
855+
};
856+
857+
server.inject('/', function (res) {
858+
859+
expect(res.statusCode).to.equal(200);
860+
});
861+
});
862+
735863
it('handles invalid log data object stringify', function (done) {
736864

737865
var handler = function (request, reply) {
@@ -978,33 +1106,6 @@ describe('Request', function () {
9781106
done();
9791107
});
9801108
});
981-
982-
it('emits request-error when view file for handler not found', function (done) {
983-
984-
var server = new Hapi.Server({ debug: false });
985-
server.connection();
986-
987-
server.views({
988-
engines: { 'html': Handlebars },
989-
path: __dirname
990-
});
991-
992-
server.once('request-error', function (request, err) {
993-
994-
expect(err).to.exist();
995-
expect(err.message).to.contain('View file not found');
996-
done();
997-
});
998-
999-
server.route({ method: 'GET', path: '/{param}', handler: { view: 'noview' } });
1000-
1001-
server.inject('/hello', function (res) {
1002-
1003-
expect(res.statusCode).to.equal(500);
1004-
expect(res.result).to.exist();
1005-
expect(res.result.message).to.equal('An internal server error occurred');
1006-
});
1007-
});
10081109
});
10091110

10101111
describe('timeout', { parallel: false }, function () {

‎test/response.js

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ var Stream = require('stream');
44
var Bluebird = require('bluebird');
55
var Boom = require('boom');
66
var Code = require('code');
7+
var Handlebars = require('handlebars');
78
var Hapi = require('..');
89
var Hoek = require('hoek');
910
var Lab = require('lab');
@@ -843,6 +844,36 @@ describe('Response', function () {
843844
});
844845
});
845846

847+
describe('_marshal()', function () {
848+
849+
it('emits request-error when view file for handler not found', function (done) {
850+
851+
var server = new Hapi.Server({ debug: false });
852+
server.connection();
853+
854+
server.views({
855+
engines: { 'html': Handlebars },
856+
path: __dirname
857+
});
858+
859+
server.once('request-error', function (request, err) {
860+
861+
expect(err).to.exist();
862+
expect(err.message).to.contain('View file not found');
863+
done();
864+
});
865+
866+
server.route({ method: 'GET', path: '/{param}', handler: { view: 'noview' } });
867+
868+
server.inject('/hello', function (res) {
869+
870+
expect(res.statusCode).to.equal(500);
871+
expect(res.result).to.exist();
872+
expect(res.result.message).to.equal('An internal server error occurred');
873+
});
874+
});
875+
});
876+
846877
describe('_streamify()', function () {
847878

848879
it('returns a formatted response', function (done) {

0 commit comments

Comments
 (0)