search

laravel / reverb

Laravel Reverb provides a real-time WebSocket communication backend for Laravel applications.
https://reverb.laravel.com
MIT License
968 stars 63 forks source link

Potential Issue with Pusher Ping/Pong Messages #224

Open sundayz opened 2 weeks ago

sundayz commented 2 weeks ago

Reverb Version

1.0.0-beta14

Laravel Version

10.10

PHP Version

8.1

Description

If I am understanding the Pusher protocol correctly, then if the WebSocket implementation supports native ping/pong messages, then Pusher will use those, and the pusher:ping and pusher:pong messages are only used if the client doesn't support websocket protocol ping messages. Link to Pusher docs here

It seems like Laravel Reverb is ONLY sending pusher:ping and not using the websocket native ping messages to determine if a connection is still alive. I found this to be a compatibility issue with the official dotnet Pusher client. If I understand the situation correctly, because the C# implementation of the websocket client handles ping messages at the ws protocol level, they didn't implement any handlers for pusher:ping messages, which makes this client incompatible with Reverb.

If my assumptions are correct then this means Reverb should detect the client version and use a different pinging strategy based on the version? Or in my case could I just disable the reverb ping messages entirely and rely on the websocket pings being handled internally to keep the connection alive?

Edit: This excerpt from the Pusher docs linked above seems to imply that the C# client's behaviour is fine:

If the WebSocket draft supports protocol level ping-pong, then on receipt of a ping message, the client MUST respond with a pong message.

If the client does not support protocol level pings and advertises (on connect) that it implements a protocol version >= 5 then the client MUST respond to a pusher:ping event with a pusher.pong event.

It doesn't explicitly state that clients that already support protocol level pings must ALSO respond to pusher:ping events.

Steps To Reproduce

Set up a minimal Reverb server and minimal Pusher dotnet client (https://github.com/pusher/pusher-websocket-dotnet), wait 1 minute after connection, server drops client connection due to missed ping and client reconnects.

sundayz commented 2 weeks ago

So websocket ping/pong is handled here: https://github.com/laravel/reverb/blob/838387607b9223b67eee592a916b36900374bb14/src/Servers/Reverb/Connection.php#L84

Maybe this should call touch() on the Pusher connection to reset lastSeenAt, and then the pusher:ping fallback won't be necessary?

drjamesj commented 2 weeks ago

I agree with above approach, probably receiving any control frame should touch() the connection the same way any non-control frame does? But to fully meet the spec, sending a ping() or pong() should also attempt to send the control frame op-code as well?

Lastly, while playing around to check this, I found that PingInactiveConnections occurs on a fixed 60 second interval on the $loop but my hunch is that this should respect the configured ping_interval value? Otherwise it seems possible that active connections may be incorrectly treated at inactive.

https://github.com/laravel/reverb/blob/838387607b9223b67eee592a916b36900374bb14/src/Servers/Reverb/Console/Commands/StartServer.php#L91-L97

joedixon commented 2 weeks ago

I don't think calling touch() on receipt of the control frame fully solves the issue since the dotnet client doesn't respond to pusher:ping so Reverb will never see a response from the client when pinging inactive connections and ultimately terminate the connection.

I'll take a look to see what's involved in adding support for this.

I notice there are some other similar issues on the repo so wonder if this should be addressed by the client. Doesn't look, at first glance, that there are any checks for the advertised supported pusher protocols.

drjamesj commented 2 weeks ago

Yes but by calling touch() in response to a control frame event would update the connection's lastSeenAt and it wouldn't be included in the next round of PingInactiveConnections anyway right? So this should work as long as the client is indeed sending ping events reliably.

What do you think?

sundayz commented 2 weeks ago

I notice there are some other similar issues on the repo so wonder if this should be addressed by the client. Doesn't look, at first glance, that there are any checks for the advertised supported pusher protocols.

I believe that because the dotnet WebSocket client supports protocol level pings they didn't bother to include support for pusher:ping messages, since the spec states you can use either method for detecting inactive connections. This is likely an issue with more client implementations out there. You don't need to check the advertised pusher version to know if ping is supported, you check the Sec-Websocket-Version header on conection (I think)