[Enhancement] Websocket API should allow sending pongs (and possibly pings)

[0x00002a]

In order to keep a websocket session alive, the usual method is to have the server periodically send "pong" frames (done in beast via async_pong). I suggest adding a send_pong method to websocket::connection that would facilitate this behaviour.

imo doing it automatically stretches the "lightweight wrapper" definition a bit, but I'm not particularly opposed to it. nvm theres literally an option for it built into beast

[Tectu]

How does the pong sent by the server show up on the client? Is this transparent to the user? i.e. would this trigger the on-read handler on the client?

[0x00002a]

I think its implementation defined, but for beast iirc its transparent. There is this setting which I found for beast, which we should probably expose (automatically sends pings on timeout).

I'm a little out of my depth here, gonna be honest :p

[Tectu]

imo doing it automatically stretches the "lightweight wrapper" definition a bit, but I'm not particularly opposed to it.

If this is completely transparent to the user (i.e. the pongs don't show up on the on-read handler (or if we can just filter them manually before the user's on-read handler gets invoked) I'd say that this is a very necessary addition to malloy. The main focus lies on usability/practicality. In many cases this results in keeping it lightweight but something like this I deem as frequently-enough-used to support it (if not even enabling it by default). I would even go as far as adding state change callbacks. One of my application talking over websocket will need the ability to report to the rest of the application (and the user) if the connection tot he server (also using malloy) has been lost. I'd argue that this is a rather common practice/need/requirement.

I'm a little out of my depth here, gonna be honest :p

Currently I'd also not appoint myself as qualified enough to make this decision. It might be worth asking over at beast.

[0x00002a]

I would even go as far as adding state change callbacks

Like the JS websockets? With onclose, onerror? I've been thinking about this, would ideally expose some sort of "event" type that could have multiple handlers registered to it imo.

[Tectu]

Yeah, exactly.

[0x00002a]

Well after implementing the ability to set the keep_alive_ping setting manually, and finding that still didn't fix the issue, then discovering that timeout::suggested(server) sets that to true anyway, I went down a bit a of a rabbit hole and found this in the websocket timeout docs:

The timeouts on the websocket stream are incompatible with the timeouts used in the tcp_stream. When constructing a websocket stream from a tcp stream that has timeouts enabled, the timeout should be disabled first before constructing the websocket stream, as shown.

(docs). I then found this: https://github.com/Tectu/malloy/blob/bf84707456ccedc06b735bcccbe8a6b8fdac2915/lib/malloy/server/http/connection.hpp#L190

^^ That sets a 30 second timeout on the socket which is never changed and which I'm guessing is untouched by beast. Interestingly I kept getting websocket timeouts after 30 seconds...

tl;dr expires_never() needs to be called on the socket before we handoff to the websocket connection. I'll make a PR shortly :p.

This issue may still be valid, but I don't have a use-case for it and I can't think of one off the top of my head. Also I'm not sure if we should add the option to disable or enable automatic pinging? Note that it has no effect if a timeout is not set (as is default for the client), and is enabled already for the server by default.

[Tectu]

Well, that was an interesting rabbit hole for sure 🥇

In the new configuration, what happens if there's an active connection between the client & the server and then the client suddenly looses network. This change would mean that the TCP layer never times-out and therefore the connection on the server would stay alive indefinitely (until the process is killed) - am I wrong here?

My gut feeling tells me that the better way of doing this is to keep an expiry timeout on the TCP socket/layer and have the automatic ping-pong system setup so the timeout doesn't get triggered as long as both the client and the server can talk to each other.

[0x00002a]

This change would mean that the TCP layer never times-out and therefore the connection on the server would stay alive indefinitely (until the process is killed) - am I wrong here?

The websocket layer added by beast has its own timeout system. The suggested settings for the server put that at 300 seconds (5m) with automatic ping pong at half of that if there is no activity, although the docs are a bit vague on whether it's half or two timeouts with the first sending the ping.

So to answer your question. It should time out after 5 minutes... Or maybe 10 but it will time out :p

[Tectu]

Closing this as this was seemingly resolved.