Skip to content
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

Make xhr transports resilient to onmessage errors #564

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jcheng5
Copy link

@jcheng5 jcheng5 commented Sep 8, 2021

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.

Without this fix, once a jsonp-polling message handler throws an
uncaught exception, the receiver stops polling.
@brycekahle
Copy link
Contributor

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
Copy link
Author

jcheng5 commented Sep 23, 2021

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
Copy link
Author

jcheng5 commented Oct 11, 2021

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

@aronatkins
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate messages emitted after error in onmessage
3 participants