Errors from decoder/encoder not handled properly

[evilworm]

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:

• Device: iPhone 13 Pro Max
• OS: ios 15.6.1 running on Angular PWA (safari engine)

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.

[darrachequesne]

Thanks a lot for the detailed issue 👍

This should be fixed by c597023, included in version 4.5.2.

[tybruffy]

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 var.length in the ack callback from an emit on the client side, where var was unintentionally null. Instead of seeing the error " Cannot read properties of null (reading 'length')" in my console like I would have before upgrading, the error is silenced and the socket disconnects and reconnects.

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?

[darrachequesne]

@tybruffy hmm, it seems you are right. On the server-side, the onpacket method is wrapped with process.nextTick(), so any error thrown in a listener is not caught by the try ... catch:

process.nextTick(function () {
  socket._onpacket(packet);
});

Source: https://github.com/socketio/socket.io/blob/10fa4a2690fafcf9415e49aad507394e0b9a9ab0/lib/client.ts#L288-L290

[mhio]

The error handler added in c597023 is masking regular errors in socket.on('x') functions for me too.

Why is the caught error e being discarded? The close/disconnect sounds a valid mechanism but can the e be emitted as an error? or least be attached to the close reason?

Although in the broader sense, the change introduced to the default error handling behaviour might not be just a point release.

[darrachequesne]

This should be fixed by 2403b88, included in version 4.5.3.

[aovchinn]

 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)

should it just clear this.reconstructor on reconnection/connection loss ?