From c2ae39664695b5e7869dc4fdc2240d440ec99680 Mon Sep 17 00:00:00 2001
From: Felix Lange <fjl@twurst.com>
Date: Thu, 11 Oct 2018 13:16:11 +0200
Subject: [PATCH] rpc: fix client shutdown hang when Close races with
 Unsubscribe

Fixes #17837
---
 rpc/client.go      | 19 ++++++++++++-------
 rpc/client_test.go | 24 ++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/rpc/client.go b/rpc/client.go
index 5343a74092..6254c95ffd 100644
--- a/rpc/client.go
+++ b/rpc/client.go
@@ -117,7 +117,8 @@ type Client struct {
 
 	// for dispatch
 	close       chan struct{}
-	didQuit     chan struct{}                  // closed when client quits
+	closing     chan struct{}                  // closed when client is quitting
+	didClose    chan struct{}                  // closed when client quits
 	reconnected chan net.Conn                  // where write/reconnect sends the new connection
 	readErr     chan error                     // errors from read
 	readResp    chan []*jsonrpcMessage         // valid messages from read
@@ -191,7 +192,8 @@ func newClient(initctx context.Context, connectFunc func(context.Context) (net.C
 		isHTTP:      isHTTP,
 		connectFunc: connectFunc,
 		close:       make(chan struct{}),
-		didQuit:     make(chan struct{}),
+		closing:     make(chan struct{}),
+		didClose:    make(chan struct{}),
 		reconnected: make(chan net.Conn),
 		readErr:     make(chan error),
 		readResp:    make(chan []*jsonrpcMessage),
@@ -228,8 +230,8 @@ func (c *Client) Close() {
 	}
 	select {
 	case c.close <- struct{}{}:
-		<-c.didQuit
-	case <-c.didQuit:
+		<-c.didClose
+	case <-c.didClose:
 	}
 }
 
@@ -429,7 +431,9 @@ func (c *Client) send(ctx context.Context, op *requestOp, msg interface{}) error
 		// This can happen if the client is overloaded or unable to keep up with
 		// subscription notifications.
 		return ctx.Err()
-	case <-c.didQuit:
+	case <-c.closing:
+		return ErrClientQuit
+	case <-c.didClose:
 		return ErrClientQuit
 	}
 }
@@ -464,7 +468,7 @@ func (c *Client) reconnect(ctx context.Context) error {
 	case c.reconnected <- newconn:
 		c.writeConn = newconn
 		return nil
-	case <-c.didQuit:
+	case <-c.didClose:
 		newconn.Close()
 		return ErrClientQuit
 	}
@@ -482,8 +486,9 @@ func (c *Client) dispatch(conn net.Conn) {
 		requestOpLock = c.requestOp // nil while the send lock is held
 		reading       = true        // if true, a read loop is running
 	)
-	defer close(c.didQuit)
+	defer close(c.didClose)
 	defer func() {
+		close(c.closing)
 		c.closeRequestOps(ErrClientQuit)
 		conn.Close()
 		if reading {
diff --git a/rpc/client_test.go b/rpc/client_test.go
index 4f354d389e..a8195c0af8 100644
--- a/rpc/client_test.go
+++ b/rpc/client_test.go
@@ -323,6 +323,30 @@ func TestClientSubscribeClose(t *testing.T) {
 	}
 }
 
+// This test reproduces https://github.com/ethereum/go-ethereum/issues/17837 where the
+// client hangs during shutdown when Unsubscribe races with Client.Close.
+func TestClientCloseUnsubscribeRace(t *testing.T) {
+	service := &NotificationTestService{}
+	server := newTestServer("eth", service)
+	defer server.Stop()
+
+	for i := 0; i < 20; i++ {
+		client := DialInProc(server)
+		nc := make(chan int)
+		sub, err := client.EthSubscribe(context.Background(), nc, "someSubscription", 3, 1)
+		if err != nil {
+			t.Fatal(err)
+		}
+		go client.Close()
+		go sub.Unsubscribe()
+		select {
+		case <-sub.Err():
+		case <-time.After(5 * time.Second):
+			t.Fatal("subscription not closed within timeout")
+		}
+	}
+}
+
 // This test checks that Client doesn't lock up when a single subscriber
 // doesn't read subscription events.
 func TestClientNotificationStorm(t *testing.T) {