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

Support keep-alive pings in GraphQlWebSocketHandler #534

Closed
rstoyanchev opened this issue Nov 14, 2022 · 5 comments
Closed

Support keep-alive pings in GraphQlWebSocketHandler #534

rstoyanchev opened this issue Nov 14, 2022 · 5 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Nov 14, 2022

Expose an additional input into GraphQlWebSocketHandler, probably Duration, that would enable periodic pings from the server to the client. This would allow detecting a lost connection or disconnected client, independent of how frequently messages are sent.

Originally requested under #528.

@brandonbyskov
Copy link

brandonbyskov commented Nov 17, 2022

@rstoyanchev, It would be useful to know the keep-alive duration and timeouts when we configure other parts of our system, such as the load balancer's idle timeout. It would be useful to be able to either calculate or configure the maximum length of time we could expect a websocket connection to stay idle before being closed. It seems like there would be 2 relevant durations:

  • Maximum length of an idle period before a server keep-alive ping is sent to client. Whether the ping is sent at a regular time interval or some time from last know activity.
  • Timeout duration that the server would wait for a client response before closing the websocket.

@rstoyanchev
Copy link
Contributor Author

rstoyanchev commented Nov 21, 2022

@brandonbyskov, good point that we can skip PINGs when the session is already sending messages. For the idle read and write timeout, I was thinking that we could be more generous with read timeouts to begin with, e.g. 2 or 3 times the keep-alive period, in which case a single keep-alive duration should do. Do you agree?

@rstoyanchev rstoyanchev modified the milestones: 1.1.0, 1.1 Backlog Nov 21, 2022
@brandonbyskov
Copy link

@rstoyanchev That would be good. As long as the timeout or how it's calculated is documented somewhere, so that we can calculate the maximum potential idle period.

@rstoyanchev rstoyanchev modified the milestones: 1.2 Backlog, 1.x Backlog May 15, 2023
@rstoyanchev rstoyanchev modified the milestones: 1.2 Backlog, 1.3 Backlog Oct 13, 2023
@sgrannan
Copy link

I just wanted to add that my company recently migrated from graphql-java-kickstart and that library did include functionality for sending KA messages to the client. Finding that this functionality is missing from this library as its now been expected to be the replacement is causing a bit of a headache for us with just the standard OOTB implementation. It seems there are Websocket connections not getting cleaned up where they previously were being closed by the kickstart library after a certain period.

It looks like you have slated this for version 1.3, but do you have any suggestions in the mean time for custom code stop-gap?

@rstoyanchev
Copy link
Contributor Author

rstoyanchev commented Mar 26, 2024

@sgrannan, looking at the auto-config, you could replace the GraphQlWebSocketHandler bean, and extend afterConnectionEstablished to get access to the WebSocketSession for sending. You'll probably also need to wrap the session, for example with ConcurrentWebSocketSessionDecorator, to protect from concurrent sending of pings vs regular messages.

@rstoyanchev rstoyanchev modified the milestones: 1.3 Backlog, 1.3.0-RC1 Apr 11, 2024
bclozel added a commit to spring-projects/spring-boot that referenced this issue Apr 11, 2024
As of spring-projects/spring-graphql#534, Spring for GraphQL supports
the configuration of keep-alive PINGs for WebSocket connections.
This commit auto-configures this value in the `GraphQlWebSocketHandler`
WebFlux and MVC implementations if the
`spring.graphql.websocket.keep-alive` property is configured.

Closes gh-40320
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants