fix: better websocket errors #265
Conversation
ghost
added
the
| if (err.description && err.description.code === 'ENOTFOUND') { | ||
| const hostname = err.description.hostname | ||
|
|
||
| err = Object.assign(new Error(`WebSocket connection failed on ${hostname}`), { |
There was a problem hiding this comment.
I don't think this pertains in the switch - which is transport agnostic. If anything, this should be emitted by the transport itself.
There was a problem hiding this comment.
Makes sense. I should probably close this PR and put this into js-libp2p-websocket-star then?
There was a problem hiding this comment.
Since the error is specific to websockets it makes sense for that to exist in js-libp2p-websocket-star and not here. I think there are two things to take into account here.
-
This PR looks like you're just trying to get more meaningful errors logged/thrown when things break to get better information to developers. If that's the case, creating that error in websocket-star and letting it propagate through libp2p-switch to the user should be sufficient for the short term.
-
The larger issue this brings up is better error messaging and handling in general. Right now, a lot of errors coming out of switch via the transports aren't terribly helpful and can be really hard to debug. I think what I'd like to see happen for this is to create the generic, extendable error types in https://github.com/libp2p/interface-connection and then have each transport catch and add meaningful metadata to those, before passing them on. This would enable switch, and other libp2p layers to handle the generic errors and take action accordingly, while giving users more detailed metadata from specific transports. If the error is meant to be fatal we can handle that appropriately. When the more common scenario of a bad connection occurs, we can simply terminate that connection and move on. Combining this with a move of switch to a state machine would be really helpful in making this a more resilient, debuggable system. cc: @diasdavid
There was a problem hiding this comment.
Thanks for the input @dryajov and @jacobheun. I'll close this one.
@jacobheun I'm going to open a more generic issue about error handling and daemon stability and it would be cool to have your second point there for an open discussion.
This updates the error when a failed WebSocket connection occurs.
A first step to fix ipfs/js-ipfs#1326.
As stated in that issue, a failed connection should not crash the daemon, we should try to reconnect from time to time.