Restart the WS connection if no pongs are received

[r-vdp]

With this change, the server will actually respond to ping frames with a corresponding pong frame. We keep track of the outstanding pings on the client side and we restart the connection if we have more than 3 outstanding pings.

The pongs are sent back automatically by the fastwebsockets library, but to do so, the WebsocketTunnelRead struct needs to have access to the WriteHalf of the WS connection, in order to send back the pongs. So we now keep the WriteHalf in an Arc<Mutex<_>>. The PingState that tracks the sent pings and received pongs is also shared between the WriteHalf and ReadHalf.

[erebe]

Hello,

Thank you for the PR, but sadly there is no chance for this to get merged. Having to take a lock everytime we need to send websocket message, is a no-go for me.

Not telling you that this is going to be merged, if it changes, but here some comments:

• You can remove the Mutex around PingSate if you use AtomicU8 internally, which will be better thant a lock
• You can avoid the mutex around the writer half if you use a channel to transmit when it needs to send pong to the writer tasks
• Also you are using async mutex, where it is correct for the Writer half, you would have been better with a sync mutex for the PingState https://docs.rs/tokio/latest/tokio/sync/struct.Mutex.html#which-kind-of-mutex-should-you-use

[r-vdp]

Ok, fair point. I will see if I can implement it without the locking.

Currently wstunnel always hangs for me when my laptop comes out of suspend, and I need to manually restart the service for it to work again, so I wanted to have a good way to be able to detect connection loss and restart the connection.

[r-vdp]

Thanks a lot also for the pointers, I'm pretty new still to the rust async stuff, so it's great to get some feedback from you.

[r-vdp]

I had a shot at implementing this without locking, I pushed a new commit.

I made the ping state no longer shared, only the writer now holds that state, and there is a channel between the two halves to inform the writer of any ping messages received.

This also allows us to send confirmation messages when the tunnel closes, as per the spec.

~~And while writing this, I'm realising that it might be better to keep the ping state in the reader instead of in the writer, as it would eliminate one message type. I can change that tomorrow.~~ It actually doesn't work like that because then the writer cannot increase the ping seq anymore.

[erebe]

finish the DNS PR first, I will review this one after :wink:

[erebe]

Going to check it next week, don't have a big chunk of spare of time those coming days

[erebe]

Hi back,

I add some time to look more into it, and indeed the change is not trivial. First, we need to disable the auto_pong ws.set_auto_pong(false) because it forces to know that we need to send a pong in the frame_reader which cause issue with the borrow.

It will allow handling the ping here, where self is already a &mut instead of doing shenanigan to handle it in frame_reader

2. We need to add a way for the WebsocketTunnelRead and WebsocketTunnelWriter to exchange information together. Using a Arc with some atomic for the ping drift, and a channel to the ping payload. When the read part receives a pong, it decrements the ping_drift, and when it receives a ping, it sends in the channel the ping payload.

I only have added an atomic in the shared state, but it would need to get a channel too.

3. Add in tunnel writer a method that return a future that you can await in the big select!

[r-vdp]

Thanks for your feedback. I'm leaving on holidays for 2 weeks and didn't have time to look at this again yet. I will get back to it once I'm back from holidays.

[erebe]

I need to refacto a bit the stuff, but this commit should do the stuff https://github.com/erebe/wstunnel/commit/f55643550b16ccbb6e8a1211a53654256e83c082

[r-vdp]

Ok, I'll close this PR then, and I'll test the changes with the new release, thanks!