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

Fix JS websocket throws an ISE on error on Safari and Firefox #5670

Merged
merged 1 commit into from Mar 4, 2024

Conversation

baconz
Copy link
Contributor

@baconz baconz commented Mar 1, 2024

We're seeing the error event fire when the websocket closes as a result of a network error on Safari and Firefox. Since the socket has already been opened, the coroutine has already been resumed and this throws an IllegalStateException ("Already resumed").

I can submit this same patch up to ktor, since I think that's where this code came from originally.

We're seeing the error event fire when the websocket closes as a result
of a network error on Safari and Firefox. Since the socket has already
been opened, the coroutine has already been resumed and this throws an
IllegalStateException ("Already resumed").
Copy link

netlify bot commented Mar 1, 2024

👷 Deploy request for apollo-android-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit ae01091

Copy link
Contributor

@BoD BoD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍 Thanks!

Copy link
Contributor

@martinbonnin martinbonnin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

Also looks like that code is still in Ktor indeed, a PR there would be great!

@martinbonnin
Copy link
Contributor

martinbonnin commented Mar 1, 2024

PS: how hard would it be to add a test for that? I think KGP can do firefox tests with Karma?

@baconz
Copy link
Contributor Author

baconz commented Mar 1, 2024

I can take a stab at adding a test over the weekend!

@baconz
Copy link
Contributor Author

baconz commented Mar 4, 2024

Hmm. I'm afraid I'm a bit stumped on this one. The challenge is that you need a websocket server running outside of the browser that dies (uncleanly) after a connection has been established.

I think the easiest way to do this would be to have a little node script that runs the websocket server, which you start up beforehand (eg a shell-based gradle task with a dependsOn), and then kill it on a timer. That feels a little brittle, but I think it would work. I found this old gradle discussion with some code that purports to do this using an old Gradle plugin to manage the process.

Can you think of a better way? I found this very old Karma plugin, but I don't see an easy way to modify KotlinKarma to allow for custom plugins.

@martinbonnin
Copy link
Contributor

martinbonnin commented Mar 4, 2024

Urg, right. No mock server in the browser :/. Not sure it's worth the extra complexity then, especially since the fix is solid. I'm merging this as is as we can revisit browser tests that need a MockServer next time. Thanks for looking at it!

@martinbonnin martinbonnin merged commit 36c152b into apollographql:main Mar 4, 2024
5 checks passed
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.

None yet

3 participants