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

Add a failing test that a Librato failure does not crash the client #32

Closed
wants to merge 2 commits into from

Conversation

kevinburkeshyp
Copy link

If Librato returns a server error, the background worker crashes. This is probably not desired behavior, but I'm not sure what it should do instead. I added a failing test to expose this.

I'm a little confused about the mechanism as well, since librato.flush() with zero arguments makes the callback an empty function. Continuing to dig.

@bobzoller
Copy link
Contributor

this just bit us too. it's a node-ism -- if an EventEmitter emits an error and nobody's listening, it throws an uncaught exception. @adborden thoughts?

@adborden
Copy link
Contributor

Thanks for the test cases @kevinburkeshyp

Been thinking about this a bit. I don't think under any circumstance would the consumer want their app to crash in the event of a Librato error, so I think it makes sense to have a default error handler. I suppose logging out the error via console.log or console.error would be the right thing to do in the default case.

I also think it's a little weird to be using EventEmitter to expose the error handling[#33]. I would prefer if error handling is more explicit. One idea would be to add a addErrorHandler method which accepts an error callback and replaces librato-node's default error handler. That would be explicit, avoid the EventEmitter, and allow us to have a default error handler in case the consumer doesn't set one.

@kevinburkeshyp
Copy link
Author

@adborden Sounds good to me

@joshperry
Copy link

Should be able to take advantage of node domains to easily fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants