-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Errors from decoder/encoder not handled properly #1551
Comments
The decoder can throw an error when trying to decode an invalid payload sent by the server, so the manager will now catch it, close the connection and then reconnect instead of crashing. Related: - socketio/socket.io#4392 - #1551
Looks like this may have had some unintended side effects? Recently upgraded from 4.5.1 and suddenly stopped seeing errors in my browser console. This is causing errors to fail silently with no way to track them down? I had a check against a Seems like we should still have the ability to see the output of the error instead of just silently passing "parse error" to the close function, no? |
@tybruffy hmm, it seems you are right. On the server-side, the onpacket method is wrapped with process.nextTick(function () {
socket._onpacket(packet);
}); |
The error handler added in c597023 is masking regular errors in Why is the caught error Although in the broader sense, the change introduced to the default error handling behaviour might not be just a point release. |
should it just clear |
Describe the bug
Socket.io-client does not handle errors thrown by default parser "socket.io-parser". When parser throws an error, there is no event fired (disconnected, connect_error, etc). The worst case scenario is if you have a binary data being transfered and the decoder has "this.reconstructor" set to true, but receives a string for some reason... the connection goes into limbo state and never recovers.
This does not happen when using for example msgpack parser since it does not throw any errors at all.
Also this does not seem to be an issue with socker.io-parser since its completely legit that the parser throws an error.
To Reproduce
You can manually throw an error in manager.js where "this.decoder.add(data);" line is and watch that you don't get any error emitted, but the console still shows the error.
Socket.IO client version:
4.5.1
Expected behavior
Disconnect the socket on decoder error, emit an error and auto-reconnect.
Also this seems to be fixed in socket.io server code - the decoder.add part is wrapped with try/catch block
Platform:
Additional context
Actual real life scenario when this happens: Angular PWA app running on iPhone ... socketio connection receives mpeg-ts binary data. The binary data is split into two packets, the decoder tries to reconstruct it. When you swipe away from the application, system will put app to sleep in about a few seconds without disconnecting the socket. When the application wakes up, the connection is lost and socket is reconnected, however the decoder still might expect binary data (having this.reconstructor flag set to true) and throws an error "got plaintext data when reconstructing a packet" which is not handled by socket.io properly and the connection stalls.
Unfortunately forceNew flag has no effect in pwa app since the object is not re-created when app wakes up.
The text was updated successfully, but these errors were encountered: