-
Notifications
You must be signed in to change notification settings - Fork 94
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
Emitting 'error'-event #56
Comments
Actually this is how Node.js works. |
@johnnycrab: independent of the |
I'm on current @matteocontrini I know that this is how node works, but I still thought it might be worth mentioning in the docs – as it is a pretty self-contained library, people like me don't think about listening to the socket error events (especially as it is emitted before the reconnection retries). I would at least add a line to the examples or something like that. I think it wouldn't hurt (–it would have helped people like me and some others on stackoverflow), but if not, nevermind and close. |
So I've come across this as well. This is the worst to debug. Without @matteocontrini: Are you seriously suggesting to keep your "Usage" example? This will crash the whole Node process in case something goes wrong. For the record, there is nothing wrong with emitting your errors on your own stream. Or publish a library with a streaming API. But this is no streaming API. It's a drop-in replacement for a logging transport. This needs at least documentation. Also examples that don't crash the process would probably be a good idea. |
I submitted a PR that tries to be a bit more forgiving and clear with connection errors ( #61 ). Would this partially address this issue? |
FYI, @dmiddlecamp's #61 is now in |
@dmiddlecamp's #61 has been released to NPM in |
There is this line 228 in the main file in
onErrored
:self.emit('error', err);
As the error is emitted, this causes the whole process to end, if the error isn't listened on elsewhere. This can lead to server crashes if Papertrail cannot be connected to. I thought maybe this should be mentioned in the docs. :)
The text was updated successfully, but these errors were encountered: