From 314f152a96bd369e258239fcba015d185cd79977 Mon Sep 17 00:00:00 2001
From: Dmitrii Okunev <xaionaro@dx.center>
Date: Fri, 25 Oct 2024 15:34:24 +0100
Subject: [PATCH] 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 0442e5b5ecbda6b567535656cfbc201c18425481.

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
---
 api/client.go | 15 +++++++++++++--
 client.go     | 24 ++++++++++++++++--------
 2 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/api/client.go b/api/client.go
index 7606873..bb0c9eb 100644
--- a/api/client.go
+++ b/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
diff --git a/client.go b/client.go
index efe20ab..b7bf39d 100644
--- a/client.go
+++ b/client.go
@@ -133,15 +133,9 @@ func (c *Client) Disconnect() 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.Disconnected)
 	})
 }
 
@@ -157,7 +151,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,
@@ -211,6 +205,20 @@ func (c *Client) connect() (err error) {
 	go c.handleRawServerMessages(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)
 	}()