search

sockjs / sockjs-client

WebSocket emulation - Javascript client
MIT License
8.45k stars 1.3k forks source link

Make xhr transports resilient to onmessage errors #564

Open jcheng5 opened 3 years ago

jcheng5 commented 3 years ago

Fixes #563

Update Sept 22, 2021: Discovered the problem was not fixed for jsonp-polling; that's fixed now as well. Also added the relevant unit tests for both xhr and jsonp.

brycekahle commented 3 years ago

Looking around the code base there are so many places we emit and don't attempt to handle any exceptions thrown by event handlers. I'm tempted to not robustify this library, because if the user lets an exception bubble up past their event handler, then all bets are off.

I tried looking around for what is considered best practice for this and didn't find much. Have you found anything that recommends one way or another?

jcheng5 commented 3 years ago

As a library author, I totally understand that temptation. However, my guess is that the vast majority of the events thrown and caught in this library are internal; the only events exposed to user code are "open", "message", and "close". And with this PR, empirically, all of the transports at least continue to work after a "message" handler throws. I didn't realize our product relied so much on this behavior until now--we've been using SockJS 0.3.x for (too) many years and it happens to handle these cases well.

"Robustifying" in the sense of sockjs-client catching exceptions from users' handler code has one significant downside I can think of, which is that they'll no longer behave the same as unhandled exceptions. With a true WebSocket, unhandled errors in onmessage are truly unhandled exceptions--the devtools debugger pauses, they show up in the console in red, window.onerror fires (I'm assuming), etc. That's true today for sockjs-client as well, it would be ideal if that continued. Unfortunately, that is harder code to write and maintain, but I think it should be far from impossible.

Would it help if I created a test suite to see what happens if open/message/close handlers throw, and test it across all the transports? I'm also happy to write further patches if those tests turn up any bad results.

jcheng5 commented 3 years ago

@brycekahle My offer stands here—happy to do some more work to address any concerns!

aronatkins commented 2 years ago

Just a note here -- #563 has auto-closed, but we would like a path forward for this work. We have had quite a bit of success using this patch.