Page MenuHomePhabricator

[Tunnelbroker] implement socket Heartbeats
ClosedPublic

Authored by kamil on Oct 30 2023, 7:02 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Oct 2, 1:08 PM
Unknown Object (File)
Wed, Oct 2, 1:08 PM
Unknown Object (File)
Wed, Oct 2, 1:08 PM
Unknown Object (File)
Wed, Oct 2, 1:08 PM
Unknown Object (File)
Wed, Oct 2, 1:08 PM
Unknown Object (File)
Wed, Oct 2, 1:08 PM
Unknown Object (File)
Wed, Oct 2, 7:56 AM
Unknown Object (File)
Wed, Oct 2, 6:01 AM
Subscribers

Details

Summary

Heartbeats algorithm:

  1. Tunnelbroker after receiving any message from clients starts to wait for SOCKET_HEARTBEAT_TIMOUT time interval.
  2. If there is SOCKET_HEARTBEAT_TIMOUT time without a message - send Heartbeat and start waiting again.
  3. If Heartbeat was sent and there is another SOCKET_HEARTBEAT_TIMOUT time interval without any response - consider the connection unhealthy and close the socket.

From the client side:

  1. If there is any message from Tunnelbroker start waiting CLIENT_SOCKET_HEARTBEAT_TIMOUT.
  2. If there was more than CLIENT_SOCKET_HEARTBEAT_TIMOUT from the last message consider the connection unhealthy and close the socket.

CLIENT_SOCKET_HEARTBEAT_TIMOUT should be at least 2x SOCKET_HEARTBEAT_TIMOUT

This diff:

  1. Adds new types
  2. Adds logic for heartbeats
  3. Implements tests exclusive for Heartbeats
  4. Make sure heartbeats will not interfere with different tests

Depends on D9618

Test Plan

Tests

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Oct 30 2023, 8:22 AM
services/commtest/src/tunnelbroker/socket.rs
93–114 ↗(On Diff #32496)

We should be able to easily remove the unwrap and expect

107 ↗(On Diff #32496)

Better to be explicit. If we would change the type later we want the compiler to shout at us in places where we need to change it instead of searching for all _.

112 ↗(On Diff #32496)

This should be more useful for debugging.

services/tunnelbroker/src/constants.rs
8 ↗(On Diff #32496)

Typo

services/tunnelbroker/src/websockets/mod.rs
208 ↗(On Diff #32496)

Maybe got_heartbeat_response (and switch true/false)? But up to you

243–244 ↗(On Diff #32496)

Shouldn't we also do it for the Ping/Pong?

272–273 ↗(On Diff #32496)

Weird formatting.

shared/tunnelbroker_messages/src/messages/heartbeat.rs
7 ↗(On Diff #32496)

This could probably look even like this (although I'm not sure if this would change the serde ouput)

address review

services/commtest/src/tunnelbroker/socket.rs
93–114 ↗(On Diff #32496)

I prefer to keep using expect with message in tests

services/tunnelbroker/src/websockets/mod.rs
208 ↗(On Diff #32496)

sounds better, thanks

243–244 ↗(On Diff #32496)

not sure about this, Ping/Pong are messages from one level lower layer, and what I want to achieve to make sure the connection is stable at the "business logic level"

272–273 ↗(On Diff #32496)

IDE tricked me again 😠

shared/tunnelbroker_messages/src/messages/heartbeat.rs
7 ↗(On Diff #32496)

error: #[serde(tag = "...")] can only be used on enums and structs with named fields

michal added inline comments.
services/tunnelbroker/src/websockets/session.rs
247

Same reason as before

This revision is now accepted and ready to land.Nov 3 2023, 6:53 AM

address review

services/tunnelbroker/src/websockets/session.rs
247

missed that, thanks