bugfix(client): panic: send on closed channel (#179)

* bugfix(client): panic: send on closed channel

Fix:

    panic: send on closed channel

    goroutine 751872 [running]:
    github.com/andreykaipov/goobs.(*Client).writeEvent(...)
        /home/xaionaro/.gvm/pkgsets/go1.22.1/global/pkg/mod/github.com/andreykaipov/goobs@v1.4.1/client.go:363
    github.com/andreykaipov/goobs.(*Client).handleOpcodes(0xc0020c81a0, 0xc0013846c0)
        /home/xaionaro/.gvm/pkgsets/go1.22.1/global/pkg/mod/github.com/andreykaipov/goobs@v1.4.1/client.go:338 +0x5a5
    created by github.com/andreykaipov/goobs.(*Client).connect in goroutine 751658
        /home/xaionaro/.gvm/pkgsets/go1.22.1/global/pkg/mod/github.com/andreykaipov/goobs@v1.4.1/client.go:200

Essentially by design we should close a channel only after we finished
all possible writing to it. Thus moving the close statement of channel
IncomingResponses to be called right after the writer to the channel is
finished.

* bugfix: Possible race condition in the disconnection procedure

There were three problems:
1. Instead of closing c.Disconnect we were writing a single event there.
   Thus if somebody will try to read from the channel the second time,
   they'll get blocked, and thus SendRequest will go into the `default`
   section of the `select`, which is potentially a panic if `c.Opcodes`
   is already closed.
2. The `close` statements in `markDisconnect` are executed without
   locking c.client.mutex. As a result there is a possible race
   condition that for example c.IncomingEvents is closed,
   but c.client.Disconnected is not (which may lead to a panic).
3. The place of the closure of c.client.IncomingResponses is
   confusing after 0442e5b.

The problems are now fixed:
1. Now we just close `c.Disconnect` instead of writing an event into it.
2. Now the `close` statements are made atomic via c.client.mutex.
3. Now we have a comment explaining the closure of c.client.IncomingResponses

Author: xaionaro

api/client.go

@@ -44,9 +44,20 @@ type Client struct {
 	// Ya like logs?
 	Log Logger
 
-	Disconnected chan bool
+	Disconnected chan struct{}
 
-	mutex sync.Mutex
+	mutex     sync.Mutex
+	closeOnce sync.Once
+}
+
+func (c *Client) Close() {
+	c.closeOnce.Do(func() {
+		c.mutex.Lock()
+		defer c.mutex.Unlock()
+
+		close(c.Disconnected)
+		close(c.Opcodes)
+	})
 }
 
 // SendRequest abstracts the logic every subclient uses to send a request and

client.go

@@ -144,16 +144,9 @@ func (c *Client) writeMessage(messageType int, data []byte) error {
 
 func (c *Client) markDisconnected() {
 	c.once.Do(func() {
-		select {
-		case c.client.Disconnected <- true:
-		default:
-		}
-
 		c.client.Log.Printf("[TRACE] Closing internal channels")
+		c.client.Close()
 		close(c.IncomingEvents)
-		close(c.client.Opcodes)
-		close(c.client.IncomingResponses)
-		close(c.client.Disconnected)
 	})
 }
 
@@ -169,7 +162,7 @@ func New(host string, opts ...Option) (*Client, error) {
 		requestHeader:      http.Header{"User-Agent": []string{"goobs/" + LibraryVersion}},
 		eventSubscriptions: subscriptions.All,
 		client: &api.Client{
-			Disconnected:      make(chan bool),
+			Disconnected:      make(chan struct{}),
 			IncomingResponses: make(chan *opcodes.RequestResponse),
 			Opcodes:           make(chan opcodes.Opcode),
 			ResponseTimeout:   10000,
@@ -221,7 +214,24 @@ func (c *Client) connect() (err error) {
 	authComplete := make(chan error)
 
 	go c.handleRawServerMessages(authComplete)
-	go c.handleOpcodes(authComplete)
+	go func() {
+		c.handleOpcodes(authComplete)
+
+		// we write to IncomingResponses only from one place:
+		// * c.handleOpcodes
+		// and we also read from it in:
+		// * c.client.SendRequest
+		// thus the `close` must happen only after c.handleOpcodes finished,
+		// and is desired to happen after c.client.SendRequest will stop
+		// using the channel as well (to avoid handling nil Responses).
+		//
+		// This line right here:
+		// * it is after c.handleOpcodes.
+		// * this place is reachable only after c.client.Opcodes is closed,
+		//   which is possible only when c.client.Disconnected is closed,
+		//   which means c.client.SendRequest would not write into c.client.IncomingResponses.
+		close(c.client.IncomingResponses)
+	}()
 
 	timer := time.NewTimer(c.client.ResponseTimeout * time.Millisecond)
 	defer timer.Stop()