lib/protocol: Don't block on Close (fixes #5794) (#5795)

lib/protocol/common_test.go

@@ -14,6 +14,7 @@ type TestModel struct {
 	weakHash      uint32
 	fromTemporary bool
 	indexFn       func(DeviceID, string, []FileInfo)
+	ccFn          func(DeviceID, ClusterConfig)
 	closedCh      chan struct{}
 	closedErr     error
 }
@@ -52,6 +53,9 @@ func (t *TestModel) Closed(conn Connection, err error) {
 }
 
 func (t *TestModel) ClusterConfig(deviceID DeviceID, config ClusterConfig) {
+	if t.ccFn != nil {
+		t.ccFn(deviceID, config)
+	}
 }
 
 func (t *TestModel) DownloadProgress(DeviceID, string, []FileDownloadProgressUpdate) {

lib/protocol/protocol.go

@@ -882,7 +882,11 @@ func (c *rawConnection) Close(err error) {
 		}
 	})
 
-	c.internalClose(err)
+	// Close might be called from a method that is called from within
+	// dispatcherLoop, resulting in a deadlock.
+	// The sending above must happen before spawning the routine, to prevent
+	// the underlying connection from terminating before sending the close msg.
+	go c.internalClose(err)
 }
 
 // internalClose is called if there is an unexpected error during normal operation.

lib/protocol/protocol_test.go

@@ -813,3 +813,22 @@ func TestClusterConfigAfterClose(t *testing.T) {
 		t.Fatal("timed out before Cluster Config returned")
 	}
 }
+
+func TestDispatcherToCloseDeadlock(t *testing.T) {
+	// Verify that we don't deadlock when calling Close() from within one of
+	// the model callbacks (ClusterConfig).
+	m := newTestModel()
+	c := NewConnection(c0ID, &testutils.BlockingRW{}, &testutils.NoopRW{}, m, "name", CompressAlways).(wireFormatConnection).Connection.(*rawConnection)
+	m.ccFn = func(devID DeviceID, cc ClusterConfig) {
+		c.Close(errManual)
+	}
+	c.Start()
+
+	c.inbox <- &ClusterConfig{}
+
+	select {
+	case <-c.dispatcherLoopStopped:
+	case <-time.After(time.Second):
+		t.Fatal("timed out before dispatcher loop terminated")
+	}
+}