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

Throwing inside response event breaks new events #3464

Closed
aalimovs opened this issue Mar 23, 2017 · 11 comments
Closed

Throwing inside response event breaks new events #3464

aalimovs opened this issue Mar 23, 2017 · 11 comments
Assignees
Labels
Milestone

Comments

@aalimovs
Copy link

@aalimovs aalimovs commented Mar 23, 2017

Are you sure this is an issue with hapi or are you just looking for some help?

Can reproduce in v15, v16. Can't in v14.

What are you trying to achieve or the steps to reproduce?

I've been hunting a weird production bug where hapi would stop emitting response events while still serving requests and everything else working just fine (except a growing memory footprint). I've been able to reproduce this by throwing inside a response event.

var Hapi = require('hapi');
var server = new Hapi.Server({});

server.connection({ port: 3000 });

server.start((err) => {
    if (err) { throw err }
});

server.on('response', function (request) {
    console.log('foo');
    throw 'bar';
});

What was the result you received?

In v14, sending 2 requests will result in seeing 2 console.logs and 2 errors.

In v15+, sending 2 requests will result in seeing 1 console.log and 1 error only, thus breaking any next response events from being triggered, while server carries on serving requests as normal.

What did you expect?

Expecting hapi not to silently stop emitting response events, that also affects all plugins that rely on the response event.

I assume this is related to podium introduction with v15.

Context

  • node version: 6.9.5
  • hapi version: v15, v16
  • os: Mac OS
@sirgallifrey
Copy link
Contributor

@sirgallifrey sirgallifrey commented Mar 24, 2017

I was able to reproduce this case

I also was able reproduce only using podium and then I've made a similar setup using
node's emitter which works as @aalimovs intended.

Using Podium

const Podium = require('podium');

const emitter = new Podium('e');
let count = 0;

emitter.on('e', function(data) {

    ++ count;
    console.log(count);
    throw new Error('bar');
})

try {
    console.log('emit');
    emitter.emit('e');
}
catch(e) {}

try {
    console.log('emit');
    emitter.emit('e');        
}
catch(e){}

/*
logs:
emit
1
emit
*/

Using node's emitter

const EventEmitter = require('events');

const emitter = new EventEmitter();
let count = 0;

emitter.on('e', function(data) {

    ++ count;
    console.log(count);
    throw new Error('bar');
})

try {
    console.log('emit');
    emitter.emit('e');
}
catch(e) {}

try {
    console.log('emit');
    emitter.emit('e');        
}
catch(e){}

/*
logs:
emit
1
emit
2
*/

Perhaps will be best to move this issue to hapijs/podium?

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Mar 24, 2017

Move to podium seems reasonable

@kanongil
Copy link
Contributor

@kanongil kanongil commented Mar 24, 2017

This seems simple: Throwing from an event handler has never been supported, and should not be done. If it worked for you previously, it was by coincidence.

@nlf
Copy link
Member

@nlf nlf commented Mar 24, 2017

Doesn't mean that events should stop emitting if something happens to be thrown. This is definitely a bug.

@aalimovs
Copy link
Author

@aalimovs aalimovs commented Mar 24, 2017

I agree, you can't have a node process running in a broken state accumulating un-emitted events in memory. Either handle the thrown error or completely crash the process

@kanongil
Copy link
Contributor

@kanongil kanongil commented Mar 28, 2017

My point is that there are no valid use cases for throwing in an event handler. Doing such, is very likely to make your code end in an unexpected state.

I agree that hapi might handle this better, though. But don't expect your code to start working after a fix.

@nlf
Copy link
Member

@nlf nlf commented Mar 28, 2017

Oh, sure, I don't think anyone's debating whether or not you should throw in an event handler. We all agree that bug's happen, however, and we certainly shouldn't stop emitting events if something does throw.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented May 29, 2017

What do you expect to happen when a handler throws?! I can resume event processing (next tick) and throw it again, but that seems like an odd solution.

@hueniverse hueniverse self-assigned this May 29, 2017
@aalimovs
Copy link
Author

@aalimovs aalimovs commented Jul 5, 2017

Found the culprit, I had a simple case of TypeError: Cannot read property 'toString' of undefined in edge case scenarios in one of the plugins, which then broke new response and request-internal events from emitting and left the hapi process running in a weird state.

@hueniverse, I believe it should resume event processing and throw again, same as it was in v14, rather than leaving hapi running in a weird state and serving requests, while not emitting any of the usual events.

I've added an example to hapijs/podium#18

@kanongil
Copy link
Contributor

@kanongil kanongil commented Jul 5, 2017

I believe it should resume event processing and throw again, same as it was in v14, rather than leaving hapi running in a weird state and serving requests, while not emitting any of the usual events.

That is not what it did. In v14 it also left hapi in a weird state (but in a different way).

@aalimovs
Copy link
Author

@aalimovs aalimovs commented Jul 5, 2017

@kanongil I'm not sure what state it would leave hapi@14 in, but it did resume event processing.

I have an error reporting plugin that listens to request-internal events with error & internal tags, which then fires the error to be tracked externally.

In hapi@16, because there's a TypeError in the response event, the request-internal is never emitted, therefore, the error is never logged.

Where as in hapi@14, TypeError in the response event wouldn't stop new events from being emitted, therefore, the error would be logged and acknowledged, rather than silently lost forever.

See example in hapijs/podium#18

@hueniverse hueniverse added the bug label Jul 18, 2017
@hueniverse hueniverse added this to the 16.5.0 milestone Jul 18, 2017
@hueniverse hueniverse changed the title throwing inside response event breaks new events Throwing inside response event breaks new events Jul 18, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants