Unhandled error on close

[Valansch]

Please describe the problem you are having in as much detail as possible:
My server lost connection to the internet and on_closed throw an error on emit(-1)

events.js:165               
    throw err;   

Error: Uncaught, unspecified "error" event. ([object Object])   
    at Client.emit (events.js:163:17)
    at WebSocketConnection.onError (*path*/node_modules/discord.js/src/client/websocket/WebSocketConnection.js:374:17)
    at WebSocket.onError (*path*/node_modules/discord.js/node_moduls/ws/lib/event-target.js:128:16)
    at emitOne (events.js:96:13)                                                            
    at WebSocket.emit (events.js:188:7)
    at _receiver.cleanup (*path*/node_modules/discord.js/node_moduls/ws/lib/websocket.js:211:14)
    at Receiver.cleanup (*path*/node_modules/discord.js/node_modules/ws/lib/receiver.js:535:15)
    at WebSocket.finalize (*path*/node_modules/discord.js/node_modules/ws/lib/websocket.js:206:20)
    at emitOne (events.js:96:13) 

I don't know how to reproduce the issue.

• discord.js version: 11.3.2
• node.js version: v6.11.5
• Operating system: Gentoo Linux 4.8.17-hardened-r2

[kyranet]

Handle the error event of your client.
Something simple like client.on('error', console.error); would do.

EDIT: If this is in a VoiceConnection, you should listen to VoiceConnection's error event as specified in the docs.

[Valansch]

Yes that would fix it. But am i wrong thinking this should be handled within the event?

[kyranet]

Absolutely. Errors should always be handled by the user, we are emitting them (the errors) to the error event for a reason. Each implementation for error handling is different, not everyone would like errors to be displayed with console.error because many users use external dependencies to format, colorize, display logs in one way or another, or even make custom loggers.

So we just emit the errors to those events, you handle them, and with that, we let you do whatever you want with them.

[Triforcey]

Thanks for the explanation @kyranet. Perhaps error handling should be made more clear in the docs, as the getting started guide doesn't mention it at all, though it is a crucial part of development. Especially in this case as disconnects are common even in production environments.

It is a good idea for JavaScript libraries to include a link to error handling on their front page or toolbar somewhere. A great example is Express on the toolbar under "Guide".

[appellation]

EventEmitters emitting an error event is not something specific to discord.js (it's referenced in the Node documentation) and therefore is not exactly similar to Express. However, I do agree that it might be useful to add to the guide considering that many users of discord.js are unfamiliar with Node. cc @discordjs/guides

[YellowAfterlife]

This should certainly be in "getting started" guide as it is only one extra line and honestly why would anyone want their bot to crash on what is generally a hard-to-reproduce problem (and good luck guessing what it was if you didn't have a stacktrace the first time it happened).

[iCrawl]

Yes/No.

This is depending on how far we want to go in teaching people how "node.js" works. Because this has nothing to do with us at all. This is a basic concept in event driven programming in node.js

Edit:
Where I am going with this is, if we start here where does it end? Next time someone wants to know how to work with Buffers, then someone wants to know how to do Y or Z. Most of which isn't what we should even cover.

[appellation]

If you have an issue with Node's error handling system, you're more than welcome to take it up with them. Async stack traces are an ongoing issue with Node error reporting, so I don't think you'll be breaking any new ground here.

The error event is intended as a way to report errors that occur during internal processes, such as a connection reset. There is no other way to report them, since they do not arise as a result of an end user action. Despite being generated by an internal mechanism, it is not the library's place to assume how you would prefer to handle such errors (logging, reporting, crashing, etc.) and so we pass it back to the end user. This is idiomatic in the Node ecosystem, as you can see in the docs. If you elect not to handle such errors, it is reasonable that the process should exit just like any other unhandled rejection.

Edit: original comment deleted

[Triforcey]

While @appellation brings up some good points concerning the nature of error handing in Node.js, it is not always reasonable for the default handler of an error to exit the process. When an error is common and its solution is clear it would be reasonable and even expected for it to be handled in a default handler set by the library. Of course you could then always change the behavior explicitly by simply setting your own handler.

[appellation]

The author of the comment that I was originally replying to deleted his comment here and re-posted it in #2879. I recommend continuing this discussion there.

[YellowAfterlife]

My gripe was not for Node's error handling, but lack of clarity in the doc - all it says is "Emitted whenever the client's WebSocket encounters a connection error". Given that the library handles heartbeats and reconnection loop (as per doc), it is hard to tell whether the library is going to forward all errors or not even if they are handled accordingly already.

I'd go as far as saying that disconnect and error events should have been the other way around (since running out of reconnection attempts is fatal for a bot, while reconnect isn't), but it's certainly too late to argue about that, so even just taking a paragraph to mention that normal application flow involves error->reconnecting->resume (or is it? The doc doesn't clarify if this is after reconnect or some other "resuming") cycles would already help - there's no way to anticipate Discord briefly disconnecting the websocket even if no service interruptions occur, and, as you can see by how often this issue gets mentioned, it does take a couple of these crashes to reflexively start adding error handlers to emitters even if it doesn't look like anything could go wrong.

[iCrawl]

Again, this relates back to teaching people what node event emitters are etc.
It doesn't matter if you open a readable filestream or a writable filestream or a discord "client" instance in this case, you are supposed to attach an error handler to it, that's a basic thing in node.js