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

Websocket i/o timeout fix #1973

Merged
merged 3 commits into from
Feb 22, 2022
Merged

Websocket i/o timeout fix #1973

merged 3 commits into from
Feb 22, 2022

Conversation

RobinCPel
Copy link
Contributor

Description

When configuring a ping-pong interval on a web socket, it requires each web socket connection to send pings, and receive pongs. To make sure that gqlgen receives the pongs in time, a read deadline is set on the websocket connection. This functions fine as long as the graphql-transport-ws sub protocol is used. However, when using the graphql-ws sub protocol, it causes i/o timeout errors.

The reason that there are timeout errors when graphql-ws is used with a configured ping-pong interval is quite simple; graphql-ws does not support ping-pong messages.

So, to fix this, we just disable the whole ping-pong mechanism when using graphql-ws!

What does this PR exactly do?

  • Fixes a typo (pingMesageType => pingMessageType)
  • Slightly refactors the code in the websocket_graphqlws.go file to look more like the code in the websocket_graphql_transport_ws.go file (for the sake of consistency)
  • Added two extra if statements in the websocket code that make sure that the keepAlive messages, as well as the ping messages, only get send when their corresponding sub protocol is selected (keep-alive => graphql-transport-ws, ping => graphql-ws)

Some extra details

The full server error is read tcp x.x.x.x:x->x.x.x.x:x: i/o timeout, and the full client error is invalid close code.


I have:

  • Added tests covering the bug / feature (see testing)

Not needed:

  • Updated any relevant documentation (see docs)

…t_graphqlws.go to look more like websocket_graphql_transport_ws.go for the sake of consistency.
…ges graphql-transport-ws only (and added tests for it).
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 74.294% when pulling 0ede73a on RobinCPel:websocket-io-timeout-fix into d7da5b0 on 99designs:master.

@soenkehahn
Copy link

We also just (independently of @RobinCPel) ran into this exact bug. Thanks a lot for putting up the PR, @RobinCPel! We would love to see this merged!

@RobinCPel RobinCPel mentioned this pull request Feb 18, 2022
2 tasks
@StevenACoffman StevenACoffman merged commit ffa857e into 99designs:master Feb 22, 2022
@StevenACoffman
Copy link
Collaborator

Thanks!

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.

4 participants