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

[sofasurfa]

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

[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?

[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