Fix "zombie" http output websocket connections

[buger]

At the moment http_server websocket implementation just writes to webscocket, and do not have a way to detect client disconnect proactively. More over, if client disconnects, and it fails write to it, it does not stop the handler, and continues to be inside the endless for loop, untill stream is stopped for !h.shutSig.IsSoftStopSignalled() {.

It cause issues when you have multiple clients connected/disconnected, because essentially it fanout using round robin to them, but if you have closed "zombie" connections, they will also consume events.

It fixes the issue by using websocket native mechanisms like "ping/pong" methods. Also it adds ephemeral "Read", just to immidiately detect the client shutdown.

[buger]

Those who would read it may find it strange, so here is why:

By setting pingPeriod to 90% of pongWait (i.e., (pongWait * 9) / 10), we ensure that:

1. The server sends a ping before the pongWait deadline:

   • If pongWait is 60 seconds, pingPeriod becomes 54 seconds.
   • This means the server sends a ping every 54 seconds.

2. There's a buffer period for the client to respond:

   • The client has 6 seconds (the remaining 10% of pongWait) to respond with a pong before the read deadline (pongWait) expires.

3. Avoiding premature timeouts:

   • If the pingPeriod were equal to pongWait, the read deadline might expire before the client has a chance to respond to the ping.
   • By setting pingPeriod slightly less than pongWait, we avoid this race condition.

[gregfurman]

This all makes sense! A small nit would be we should consider adding these as configuration variables with the advanced() flag. Think these are sensible defaults in any-case.

[buger]

On the question, on why we need both ping/pong and Read goroutine:

• Necessity of Ping/Pong with a Read Loop:
  • Idle Clients: The read loop alone cannot detect if an idle client has disconnected unexpectedly.
  • Timely Detection: Ping/pong can detect unresponsive clients within a predictable timeframe (pongWait), regardless of message traffic.
  • Protocol Compliance: Utilizing ping/pong aligns with WebSocket protocol standards and best practices.

[buger]

^ fixed linter

[gregfurman]

This is great! Thanks so much for the contribution and fix 🍱😄 Just a couple of questions/suggestions.

Mostly concerned that the ReadMessage in that go-routine could cause a race-condition when sending a Ping to the same socket we write the message bytes to.

Otherwise, run make fmt && make lint on your local to sort out these linting issues 🙂

[gregfurman]

This setWriteDeadline method on the connection doesn't ever return an error (despite that confusing function signature)

Suggested change

	if err := ws.SetWriteDeadline(time.Now().Add(writeWait)); err != nil {
	h.log.Warn("Failed to set write deadline for ping: %v", err)
	return
	}
	ws.SetWriteDeadline(time.Now().Add(writeWait))

[gregfurman]

Same as below comment:

Suggested change

	if err := ws.SetWriteDeadline(time.Now().Add(writeWait)); err != nil {
	writeErr = err
	break
	}
	ws.SetWriteDeadline(time.Now().Add(writeWait));

[gregfurman]

Is it necessary to set the read deadline before creating and inside the PongHandler?

[buger]

Yes, both calls are necessary:

1. Initial SetReadDeadline sets the first timeout for the connection to prevent indefinite blocking
2. PongHandler's SetReadDeadline extends the timeout each time a pong is received, keeping the connection alive only while the client is responsive

Think of it like resetting a timer - you need both the initial timer setup and the mechanism to reset it when activity is detected.

[gregfurman]

Not 100% clued up on websockets so excuse the ignorance but will doing a ws.ReadMessage() clear all bytes in the buffer after reading? Else, does a WriteMessage fan-out to all consumers on the connection?

I'm concerned about a race condition where the payload we send using the below WriteMessage ends up being consumed in this goroutine instead of to the client where intending to send this.

[buger]

There's no risk of a race condition like the one you're describing.

In the context of WebSockets, the connection between the client and the server is full-duplex, meaning both ends can send and receive messages independently of each other. This feature allows for simultaneous communication without interference.

• Separate Channels: The ws.ReadMessage() method reads messages that the client has sent to the server. The ws.WriteMessage() method sends messages from the server to the client. They operate on separate channels within the WebSocket protocol and don't interfere with each other.

• No Buffer Overlap: When you call ws.WriteMessage(), it doesn't add messages to a buffer that ws.ReadMessage() would read from. Similarly, ws.ReadMessage() doesn't consume messages that the server has sent—only those sent by the client.

• Required Read Loop: Implementing a read loop as done, is recommended in WebSocket servers. It ensures that control frames (like pings and pongs) are properly handled, which is essential for maintaining the connection. Not reading from the WebSocket can lead to the connection being closed due to unhandled control frames or buffer overflows.

[gregfurman]

This all makes sense! A small nit would be we should consider adding these as configuration variables with the advanced() flag. Think these are sensible defaults in any-case.

[buger]

@jem-davies @gregfurman sorry for delay!

Tried to address all the comments!

[buger]

@gregfurman @jem-davies just a gentle bump in case if you missed the last message :)

[buger]

Applied suggestions

[jem-davies]

am happy to merge if @gregfurman is -- thanks a lot !

-- need to change the version set in a few places now, will raise a ticket to do that instead.

[gregfurman]

LGTM! Thanks @buger for addressing the comments and @jem-davies for the extra 👀