warp icon indicating copy to clipboard operation
warp copied to clipboard

websocket `auto_ping` as parallel to sse `keep_alive`

Open codesections opened this issue 6 years ago • 16 comments
trafficstars

The SSE filter has a keep_alive method that sends periodic messages to clients to persist the connection. However, the WS filter lacks similar functionality, even though it is sometimes needed and is often included (for example, in the startAutoPing method in Cluster WS, one of the more prominent WebSockets libraries in the JavaScript ecosystem).

Would it be possible to add that functionality? If so, I'm happy to help—though I took a look at the code and am not quite sure where to start, so I'd appreciate a bit of guidance.

codesections avatar Sep 11 '19 13:09 codesections

The underlying implementation (tungstenite) supports Ping/Pong messages, but warp has created their own API which doesn't.

So either:

  • the warp API is to be rewritten to have a better coverage of the websocket protocol
  • you use hyper to upgrade http connections and switch to tungstenite yourself
  • use a websocket library (tungstenite/ws/websocket) directly on another port

Hope it clarifies things

najamelan avatar Sep 13 '19 21:09 najamelan

Thanks, that was my understanding as well.

the warp API is to be rewritten to have a better coverage of the websocket protocol

That is exactly what I am proposing. If no one objects, I can work on an initial PR that would lay the groundwork for an extension of the Warp API to allow for ping messages. I think the first step would be to give the Message struct a .ping public method, paralleling the .text and .binary methods it already exposes.

codesections avatar Sep 13 '19 21:09 codesections

You can have a look at prior thoughts here: #257 and #258

najamelan avatar Sep 13 '19 22:09 najamelan

@codesections have you started on this already? I also need either the manual way to send / receive pings and pongs or the auto_ping functionality you describe. If you haven't started working on it, I can also take a stab at implementing it.

emschwartz avatar Sep 24 '19 20:09 emschwartz

have you started on this already?

My apologies for the delay in getting back to you on this. All I have done so far is the (very small) PR in #281, which would allow users to manually send Pings

codesections avatar Oct 04 '19 01:10 codesections

@codesections have you started on this already? I also need either the manual way to send / receive pings and pongs or the auto_ping functionality you describe. If you haven't started working on it, I can also take a stab at implementing it.

Just remember that tungstenite always responds to pings automatically as long as you keep polling the stream or sending things in the sink. You should never manually answer the pings.

najamelan avatar Oct 04 '19 07:10 najamelan

Just remember that tungstenite always responds to pings automatically as long as you keep polling the stream or sending things in the sink. You should never manually answer the pings.

Right, I understand that—but the functionality I'm looking for is not about manually answering pings, it's about sending pings.

There are two general ways a client–server connection can implement a WebSocket connection with keep alive. First, the client can send pings and expect the server to answer them with pongs; Warp already supports this setup.

Second, however, the server can send pings to the client (and receive pongs in reply). Warp currently does not support that setup, and thus doesn't work with clients that expect the server to send the pings. The PR I proposed would allow Warp to work in this second scenario as well.

codesections avatar Oct 04 '19 14:10 codesections

Thanks for working on this, this would indeed be an useful feature.

jq-rs avatar Jan 22 '20 20:01 jq-rs

@codesections Sorry if this is a bit late, there is an open-standing bug on tungstenite considering handling of pong messages. It probably not affect you if you just send ping messages, it affects sending pong as unidirectional heartbeats. Just thought it best if people are aware of it: https://github.com/snapview/tungstenite-rs/issues/78

najamelan avatar Jan 23 '20 12:01 najamelan

As an update: warp now supports creating Message::ping(data), so it is possible to construct this manually.

But I do think it'd be nice if this were just an option, similar to Ws::max_send_queue and the like. A question around implementation: would an idle checker apply only to sends, or include receives as well?

seanmonstar avatar Jan 24 '20 21:01 seanmonstar

I am not sure what @codesections has in mind, but I would be happy to have a keep-alive setting that drops inactive connections. WebSocket RFC states that ping must be replied by client and server with pong and it can be sent almost at any phase.

For application level keep-alive this would mean that some kind of ping_interval could be set with inactivity_timeout which would close the connection when pongs or other data is not received within some period. Tk-http has an API example available which also includes byte-level setting. This has been working well for me earlier. Tk-http has this enabled by default.

jq-rs avatar Jan 25 '20 15:01 jq-rs

I was able to do above keep-alive setting manually with some additional modifications to Warp and simple use of atomic variables with timers (PR https://github.com/jq-rs/arki-server/pull/5) where pings and pongs are counted and if there is too much of a difference, the connection is closed as inactive.

The commit for needed Warp changes is sent via #429 for 0.1.x

jq-rs avatar Jan 30 '20 07:01 jq-rs

What's the status on this? I realised this was the cause of #798. I just need some way to tell warp to periodically ping clients.

indianakernick avatar Apr 27 '21 05:04 indianakernick

+1 here, any updates, please?

rsalmei avatar Oct 20 '22 16:10 rsalmei

Has there been any progress on this? If not, would there be any objection to me adding a ping_interval and ping_timeout, similar to the python websockets API? I'm not entirely sure where they should be set, either under some struct in this repo or under the tungstenite WebSocketConfig that this repo uses.

seemeroland avatar Sep 22 '23 15:09 seemeroland

Hey @seanmonstar, any update on this? Would you be open to an upstream change that adds ping / pong capabilities for warp websockets? Thanks!

szgupta avatar Feb 27 '24 20:02 szgupta