SetReadDeadline - Chat example (remove idle clients)

[sofasurfa]

Is there an existing issue for this?

• I have searched the existing issues

Current Behavior

I used chat example to modify some code, added clients map and send chan. The example makes sense that after pongWait the next reads return err and cause defer to be called thus calling c.hub.unregister <- c. Happy days.

**hub.go**
---

type Hub struct {
	// Registered clients.
	clients map[uuid.UUID]*Client

	// Send to specific user
	send chan Message

	// Inbound messages from the clients.
	broadcast chan []byte

	// Register requests from the clients.
	register chan *Client

	// Unregister requests from clients.
	unregister chan *Client
}

**client.go**
---
func (c *Client) readPump(env *config.Env) {
	defer func() {
		c.hub.unregister <- c
		c.conn.Close()
	}()
	c.conn.SetReadLimit(maxMessageSize)
	c.conn.SetReadDeadline(time.Now().Add(pongWait))
	// Reset pongWait after we get the pong
	// to keep client conn opened.
	c.conn.SetPongHandler(func(string) error { c.conn.SetReadDeadline(time.Now().Add(pongWait)); return nil })

for {

		err := c.conn.ReadJSON(&msg)
		if err != nil {
			if websocket.IsUnexpectedCloseError(err, websocket.CloseGoingAway, websocket.CloseAbnormalClosure) {
				env.SugaredLogger.Error("error in ws ReadJSON:", err)
			}
			break
		}
 		c.hub.send <- msg
	}
}

Expected Behavior

Let's say app user exits, and pong never comes back (thus no next read)... that means the socket is still opened (kind of blocked now but still open)? That socket/connection should be deleted for good after ReadDeadline, it's not. How can I make defer work (that will call c.hub.unregister <- c) after pongWait without waiting for the next read which might not happen. I might have missed something obvious in the code or should have another goroutine checking clients and if they have expired and remove manually? Thanks. Hopefully this will help other developers who are a little confused.
@garyburd

Steps To Reproduce

No response

Anything else?

No response

[hulkingshtick]

How can I make defer work (that will call c.hub.unregister <- c) after pongWait without waiting for the next read which might not happen.

The code in readPump looks correct. ReadJSON returns with an error when the peer sends a close message, the peer terminates the TCP connection, or when the read deadline is exceeded.

That socket/connection should be deleted for good after ReadDeadline, it's not.

We cannot determine why this is happening because you don't show the code that manages the collection of Client or connections. In any case, that's an application issue and not a websocket issue.

[sofasurfa]

@hulkingshtick thx for response, my code assumes it's the same as in the chat example.

[hulkingshtick]

I edited my previous comment to be more accurate. What specifically is the scenario where one of those conditions will not be true, but the peer is gone?

The hub code in your application is not the same as the hub code in the application. Perhaps you have a bug there.

[sofasurfa]

@garyburd @hulkingshtick thx, just re-read what you wrote, so you're saying ReadJSON will return err itself without client sending anything? It's just the comment explaining the function says otherwise ("all future reads will return an error"), that why I'm confused I guess:

// SetReadDeadline sets the read deadline on the underlying network connection.
// After a read has timed out, the websocket connection state is corrupt and
// all future reads will return an error. A zero value for t means reads will
// not time out.
func (c *Conn) SetReadDeadline(t time.Time) error {
	return c.conn.SetReadDeadline(t)
}

based on the comment, in case someone's phone battery dies or client network doesn't send a websocket close(), then ReadJSON won't return err at all even after ReadDeadline. (p.s I didn't look at the pkg code, only the comment my VsCode gave for the function)

[sofasurfa]

@garyburd @hulkingshtick would appreciate if you could confirm this "in case someone's phone battery dies or client network doesn't send a websocket close(), then ReadJSON won't return err at all even after ReadDeadline bcz a read never came through"

[sofasurfa]

@garyburd @hulkingshtick I think I found the code inconn.go line 1028 in func (c *Conn) NextReader() that return c.readErr

	// Applications that do handle the error returned from this method spin in
	// tight loop on connection failure. To help application developers detect
	// this error, panic on repeated reads to the failed connection.
	c.readErrCount++
	if c.readErrCount >= 1000 {
		panic("repeated read on failed websocket connection")
	}

	return noFrame, nil, c.readErr

which means indeed like you said, ReadJSON will return err after SetReadDeadline, supported by the comment for this function as well

// Applications must break out of the application's read loop when this method
// returns a non-nil error value. Errors returned from this method are
// permanent. Once this method returns a non-nil error, all subsequent calls to
// this method return the same error.

So maybe making ReadJSON comment more clear about err being returned after SetReadDeadline so it's more clear to avoid confusion? Or maybe updating nextReader() comment and adding info about SetReadDeadline since ReadJSON uses nextReader anyway?

[hulkingshtick]

In the Go, a read deadline in the context of network connections is a time limit set on reading from a network connection. The deadline specifies how long the program will wait for data to be received before giving up and returning an error.

[sofasurfa]

@hulkingshtick ahh see it's an internal go pkg doing the work, then chat example takes my concern into account... Thx for clarifying that