strawberry-graphql / strawberry

A GraphQL library for Python that leverages type annotations 🍓
https://strawberry.rocks
MIT License
4.01k stars 531 forks source link

Strawberry must provide server side ping messages #2992

Open XChikuX opened 1 year ago

XChikuX commented 1 year ago

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

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

Fund with Polar

XChikuX commented 9 months ago

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 commented 1 week ago

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

Not sure if you've handled this bug yet.

DoctorJohn commented 1 week ago

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.

XChikuX commented 1 week ago

@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: https://github.com/enisdenjo/graphql-ws/issues/494

DoctorJohn commented 1 week ago

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 commented 1 week ago

@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.