electrumx: improve closing and fix close panic (#192)

* electrumx: fix clientConn chan double-close

Prevent attempts to close the closeCh channel in clientConns more than
once. Attempting to close a channel that is already closed results in a
panic.

The pinger was occasionally triggering a panic by calling Close when the
connection had already been closed.

* electrumx: wait for pinger to exit before returning in Close

* electrumx: close closeCh after closing connection

* electrumx: add test for closing clientConn, fix hang in Close

Add TestClose to test closing clientConns.

Fix hang in Close caused by the clientCh being set to nil before being
read by pinger, causing pinger to never exit.

hemi/electrumx/conn.go

@@ -25,6 +25,7 @@ const (
 // clientConn is a connection with an ElectrumX server.
 type clientConn struct {
 	mx sync.Mutex
+	wg sync.WaitGroup
 
 	conn      net.Conn
 	requestID uint64
@@ -40,6 +41,8 @@ func newClientConn(conn net.Conn, onClose func(c *clientConn)) *clientConn {
 		closeCh: make(chan struct{}),
 		onClose: onClose,
 	}
+
+	c.wg.Add(1)
 	go c.pinger()
 	return c
 }
@@ -152,6 +155,8 @@ func (c *clientConn) ping() error {
 
 // pinger pings each connection on a ticker.
 func (c *clientConn) pinger() {
+	defer c.wg.Done()
+
 	ticker := time.NewTicker(connPingInterval)
 	for {
 		select {
@@ -181,6 +186,17 @@ func (c *clientConn) Close() error {
 	if c.onClose != nil {
 		c.onClose(c)
 	}
-	close(c.closeCh)
-	return c.conn.Close()
+
+	if err := c.conn.Close(); err != nil {
+		return err
+	}
+
+	if c.closeCh != nil {
+		close(c.closeCh)
+	}
+
+	// Wait for pinger to exit.
+	c.wg.Wait()
+	c.closeCh = nil
+	return nil
 }

hemi/electrumx/conn_test.go

@@ -33,9 +33,9 @@ func TestClientConn(t *testing.T) {
 	if err != nil {
 		t.Fatalf("failed to dial server: %v", err)
 	}
-	defer conn.Close()
 
 	c := newClientConn(conn, nil)
+	defer c.Close()
 
 	tests := []struct {
 		name   string
@@ -165,6 +165,33 @@ func TestReadResponse(t *testing.T) {
 	}
 }
 
+func TestClose(t *testing.T) {
+	server := createMockServer(t)
+	defer server.Close()
+
+	conn, err := net.Dial("tcp", server.address)
+	if err != nil {
+		t.Fatalf("failed to dial server: %v", err)
+	}
+
+	c := newClientConn(conn, nil)
+
+	// Ping the server.
+	if err := c.ping(); err != nil {
+		t.Errorf("failed to ping server: %v", err)
+	}
+
+	// Close the client.
+	if err := c.Close(); err != nil {
+		t.Errorf("failed to close client: %v", err)
+	}
+
+	// Close the client again. This should do nothing and return net.ErrClosed.
+	if err := c.Close(); err == nil || !errors.Is(err, net.ErrClosed) {
+		t.Errorf("failed to close client (second): %v", err)
+	}
+}
+
 type mockServer struct {
 	address string
 	ln      net.Listener