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

Emitting 'error'-event #56

Open
johnnycrab opened this issue Jun 22, 2016 · 7 comments
Open

Emitting 'error'-event #56

johnnycrab opened this issue Jun 22, 2016 · 7 comments

Comments

@johnnycrab
Copy link

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. :)

@matteocontrini
Copy link

Actually this is how Node.js works.
Refenrece.

@troy
Copy link
Contributor

troy commented Jun 23, 2016

@johnnycrab: independent of the emit question, if you encountered a case where not being able to connect to the remote syslog server affected the app, it might be worth trying the current git master commit. Although Papertrail didn't write this library, and obviously can't test or vouch for every client library's connection handling, master includes fixes to some, maybe all, cases where being unable to connect affects the app.

@johnnycrab
Copy link
Author

I'm on current master, but thanks for the note. :)

@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.

@freezy
Copy link

freezy commented Jul 13, 2016

So I've come across this as well.

This is the worst to debug. Without longjohn Node just crashes with a connection error, without any hint at all where the problem could be (stacktrace is a threeliner). This took me hours to find.

@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.

@dmiddlecamp
Copy link
Contributor

I submitted a PR that tries to be a bit more forgiving and clear with connection errors ( #61 ). Would this partially address this issue?

troy added a commit that referenced this issue Nov 7, 2016
Patch to try and address #20 / #49 / #56, peacefully log connection errors …
@troy
Copy link
Contributor

troy commented Nov 7, 2016

FYI, @dmiddlecamp's #61 is now in master and will be part of a 1.0.4 release, probably in the next week. If anyone wants to pull master and see whether they can repro this, feel free.

@troy
Copy link
Contributor

troy commented Nov 10, 2016

@dmiddlecamp's #61 has been released to NPM in 1.0.4. If anyone experiences this or believes they have, please make sure you're running >= 1.0.4; it may mitigate this issue to the point of eliminating it.

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

No branches or pull requests

5 participants