-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
Make xhr transports resilient to onmessage errors #564
base: main
Are you sure you want to change the base?
Conversation
Without this fix, once a jsonp-polling message handler throws an uncaught exception, the receiver stops polling.
Looking around the code base there are so many places we 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? |
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 "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 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. |
@brycekahle My offer stands here—happy to do some more work to address any concerns! |
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. |
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.