rpcc: Fix potential deadlock in stream Sync (#134)

* rpcc: Fix potential deadlock in stream Sync

The simplified Conn mutex handling introduced in #74 incorrectly moved
the mutex Unlock to after stream.write. This created the opportunity for
a racy mutex deadlock scenario with simultaneous writes to stream while
`Sync` is being performed.

Due to it's racyness, this was only occasionally caught by
`TestStreamSyncNotifyDeadlock` and evaded detection.

* rpcc: Improve odds of deadlock in TestStreamSyncNotifyDeadlock

rpcc/conn.go

@@ -446,12 +446,12 @@ func (c *Conn) send(ctx context.Context, call *rpcCall) (err error) {
 func (c *Conn) notify(method string, data []byte) {
 	c.mu.Lock()
 	stream := c.streams[method]
+	c.mu.Unlock()
 	if stream != nil {
 		// Stream writer must be able to handle incoming writes
 		// even after it has been removed (unsubscribed).
 		stream.write(method, data)
 	}
-	c.mu.Unlock()
 }
 
 // listen registers a new stream listener (chan) for the RPC notification

rpcc/stream_sync_test.go

@@ -2,6 +2,7 @@ package rpcc
 
 import (
 	"context"
+	"fmt"
 	"strconv"
 	"strings"
 	"sync"
@@ -81,9 +82,7 @@ func (s *fakeStream) Ready() <-chan struct{}      { return nil }
 func (s *fakeStream) RecvMsg(m interface{}) error { return nil }
 func (s *fakeStream) Close() error                { return nil }
 
-var (
-	_ Stream = (*fakeStream)(nil)
-)
+var _ Stream = (*fakeStream)(nil)
 
 func TestSyncError(t *testing.T) {
 	conn1, connCancel1 := newTestStreamConn()
@@ -149,7 +148,6 @@ func TestSyncError(t *testing.T) {
 				t.Error("Expected error, got nil")
 			}
 		})
-
 	}
 }
 
@@ -163,18 +161,25 @@ func TestStreamSyncNotifyDeadlock(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
+	defer s1.Close()
 	s2, err := NewStream(ctx, "test2", conn)
 	if err != nil {
 		t.Fatal(err)
 	}
+	defer s2.Close()
 
-	go conn.notify("test1", []byte(`{"hello": "world"}`))
-	go conn.notify("test2", []byte(`{"hello": "world"}`))
-
-	// This could cause a deadlock due to competition for same mutexes:
-	// https://github.com/mafredri/cdp/issues/90
-	// https://github.com/mafredri/cdp/pull/91
-	err = Sync(s1, s2)
+	syncErr := make(chan error)
+	go func() {
+		// This could cause a deadlock due to competition for same mutexes:
+		// https://github.com/mafredri/cdp/issues/90
+		// https://github.com/mafredri/cdp/pull/91
+		syncErr <- Sync(s1, s2)
+	}()
+	for i := 0; i < 10; i++ {
+		conn.notify("test1", []byte(fmt.Sprintf(`{"hello": "world", "count": %d}`, i)))
+		conn.notify("test2", []byte(fmt.Sprintf(`{"hello": "world", "count": %d}`, i)))
+	}
+	err = <-syncErr
 	if err != nil {
 		t.Fatal(err)
 	}