Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add WebSocket Ping/Pong #306

Closed
wants to merge 4 commits into from
Closed

Add WebSocket Ping/Pong #306

wants to merge 4 commits into from

Conversation

cohoe
Copy link
Member

@cohoe cohoe commented Jul 9, 2024

This closes #187. Since browser clients do not implement WebSocket control frames, the native ping/pong mechanism is unavailable. it is left to the client/server protocol vendors to develop their own solution. This gives native clients the ability to make smarter healthcheck functions to determine the liveness state of the socket. For example, Tricordarr currently does a simple check based on the WS state https://github.com/jocosocial/tricordarr/blob/main/src/libraries/Network/Websockets.ts#L84-L91.

It is entirely possible this is a solution in search of a problem. On-board I feel like the auto reconnection library in Tricordarr seemed to work pretty well since roaming to an area with bad network closed the socket and it attempted to reconnect a few times before giving up. But I don't have specific anecdotes to confirm or deny this.

@cohoe cohoe requested review from hendricksond and challfry July 9, 2024 04:36
Copy link
Member

@hendricksond hendricksond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment on lines 225-226 is no longer accurate.

@challfry
Copy link
Collaborator

challfry commented Jul 9, 2024

  1. From my testing, both Chrome and Safari will respond to Ping frames sent from the server.

  2. If we need the server to send control frames, ws.pingInterval = TimeAmount.seconds(60) is a better way to do it. The WebSocket implementation in Vapor has support for ping/pong control frames, including 'onPing' and 'onPong' callbacks that tell you when you when each type of control frame is received.

  3. The Notification socket does not implement the onText nor onBinary handler; if there are incoming data messages from clients on the notification socket, the server isn't configured to receive them. I couldn't find anywhere that we send any data on the Notification socket other than 'SocketNotificationData' which happens at https://github.com/jocosocial/swiftarr/blob/15a6a32736735a3c793f0a5d68666b975b4d21af/Sources/swiftarr/Helpers/WebSocketStorage.swift#L60C1-L75C1.

@challfry
Copy link
Collaborator

challfry commented Jul 9, 2024

Also, I'm not an expert on network protocols, but it appears that the WS implementations on iOS and in Safari and Chrome employ TCP keepalive, as all these clients detect when the socket has unexpectedly closed and hasn't sent a close packet (e.g. I kill the server process) and the clients detect the closure without trying to send any application-level data.

My understanding is that TCP keepalive is considered less reliable due to things like web proxies, where keepalive could only tell you that your connection to the proxy is still running but a WS-level ping would actually hit the server. I don't think this would be an issue on boat, but my past record of being correct when I think, "of course the boat's network wouldn't do that" isn't great.

@cohoe
Copy link
Member Author

cohoe commented Jul 10, 2024

I don't see any use case in the server needing to initiate sending control frames (a la ping) to clients. Server doesn't care if you're connected or not other than if you have a NotificationSocket or FezSocket within the appropriate WebSocketStorage.

Traefik is the expected termination point for any TCP connection from a client to the server while on board. Traefik to Swiftarr communication should be nearly 100% reliable. What happens between client and Traefik is where we get spicy.

Thinking out loud from the client perspective of implementing this healthcheck method, I'd need something like this:

function wsHealthCheck() {
  if socket.state != open {
    return false;
  }
  socket.sendMessage({type: ping, info: ping, contentID: abc123});
  # async/await some interval like <=5 seconds for a response
  if socket.receivedMessage.type == .pong {
    return true;
  }
  # default case should be that the socket is unhealthy
  return false;
}

In Tricordarr this likely involves some Promise() voodoo that I don't fully grok. The more I think about this the more I'm "meh" on the entire feature. If you think this could be useful I'm game to implement it. But if not I think I'm ok closing this and leaving it dormant until some future day when we have more real-boat information to inform our opinions.

@cohoe
Copy link
Member Author

cohoe commented Aug 24, 2024

I am going to close this until a more concrete need comes up.

@cohoe cohoe closed this Aug 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Websocket Ping
3 participants