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

Strawberry must provide server side ping messages #2992

Open
XChikuX opened this issue Jul 30, 2023 · 6 comments
Open

Strawberry must provide server side ping messages #2992

XChikuX opened this issue Jul 30, 2023 · 6 comments
Assignees
Labels
bug Something isn't working

Comments

@XChikuX
Copy link
Contributor

XChikuX commented Jul 30, 2023

Server side ping messages are necessary to keep the websocket connection open on all types of platforms.
The particular platform I'm working with is react-native on Android

Describe the Bug

React-Native on Android Websockets close due to no server side PING messages within 8-10 seconds.
You can follow the crux of the discussion here: https://discord.com/channels/689806334337482765/1134350180653740065
I have verified the issue with the author of graphql-ws repo.

System Information

  • Operating system:
  • Strawberry version (if applicable):

Additional Context

I would recommend sending PINGs from strawberry every 6 seconds to account for most types of client websocket timeouts.

Probably would require changes to handlers.py within these lines:

    async def handle_connection_init(self, message: ConnectionInitMessage) -> None:
        if self.connection_timed_out:
            # No way to reliably excercise this case during testing
            return  # pragma: no cover
        if self.connection_init_timeout_task:
            self.connection_init_timeout_task.cancel()

        if message.payload is not UNSET and not isinstance(message.payload, dict):
            await self.close(code=4400, reason="Invalid connection init payload")
            return

        self.connection_params = message.payload

        if self.connection_init_received:
            reason = "Too many initialisation requests"
            await self.close(code=4429, reason=reason)
            return

        self.connection_init_received = True
        await self.send_message(ConnectionAckMessage())
        self.connection_acknowledged = True

    async def handle_ping(self, message: PingMessage) -> None:
        await self.send_message(PongMessage())

    async def handle_pong(self, message: PongMessage) -> None:
        pass

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@XChikuX XChikuX added the bug Something isn't working label Jul 30, 2023
@XChikuX
Copy link
Contributor Author

XChikuX commented Feb 12, 2024

Hey @patrick91, just wanted to bring some attention to this, since its been quite a few months.

This is a core bug in the WebSocket protocol implementation which we should fix to stabilize strawberry.

@XChikuX
Copy link
Contributor Author

XChikuX commented Nov 1, 2024

@DoctorJohn Bumping this back up due to the similarty with your latest PR. #3678

Not sure if you've handled this bug yet.

@DoctorJohn
Copy link
Member

Hi @XChikuX, thanks for the ping. I haven't seen this issue before and I'm kinda surprised. I run Strawberry with WebSockets as a backend for a React Native app in production and don't have issues with it.

I was under the impression that server-sent graphql-ws pings are optional since the protocol specification does not state that they must be sent at all. Do you have a reference to the conversation with the graphql-ws author?

Personally, I use the AIOHTTP integration which keeps WebSocket connections alive by default using WebSocket ping/pong messages. Uvicorn also automatically sends ping/pong messages, however, with a 20 seconds interval by default.

@DoctorJohn DoctorJohn self-assigned this Nov 1, 2024
@XChikuX
Copy link
Contributor Author

XChikuX commented Nov 1, 2024

@DoctorJohn

The 20 second default is what closes the connections I suppose. It must be a much smaller value last I checked with the author.

I personally use hypercorn, I haven't necessarily checked their defaults myself. I was able to solve my own issue with client side pings. But that's not ideal.

Here's the conversation with the author on the issue I brought up with graphql-ws: enisdenjo/graphql-ws#494

@DoctorJohn
Copy link
Member

Ah, thanks for referencing the issue, that was very helpful. The graphql-ws author is also referring to WebSocket ping/pongs and not the graphql-ws ping/pongs. That means Strawberries implementation of the protocol in this regard is still correct.

What we should do, however, is mentioning in all integration guides that one must configure WebSocket ping/pongs for their server. I'll create a PR for this over the weekend.

FYI: Hypercorn doesn't appear to sent pings by default. However, pings can be enabled using the --websocket-ping-interval command line argument or the websocket_ping_interval config key. The documentation for this can be found on the bottom of the Hypercorn config documentation page.

@XChikuX
Copy link
Contributor Author

XChikuX commented Nov 1, 2024

@DoctorJohn Appreciate the documentation reference :)

Saved me sometime!

In regards to your suggestions:

That sounds like the right thing to do, if strawberry isn't the responsible party.

My observations are still that, the android platform closing WS connection after ~7-8 seconds of no server side pings on wss://. I haven't tested it on an apple device yet.

We could maybe make a special mention that the recommended websocket-ping-interval should be around 6000ms for a react-native development environment? Especially since the 20 seconds standard for uvicorn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants