Merge pull request #118 from embogomolov/fix/interrupt-group-deadlock-extended

common/interrupt: close connections outside group lock

common/interrupt/conn.go

@@ -29,8 +29,8 @@ type Conn struct {
 
 func (c *Conn) Close() error {
 	c.group.access.Lock()
-	defer c.group.access.Unlock()
 	c.group.connections.Remove(c.element)
+	c.group.access.Unlock()
 	return c.Conn.Close()
 }
 
@@ -58,8 +58,8 @@ type PacketConn struct {
 
 func (c *PacketConn) Close() error {
 	c.group.access.Lock()
-	defer c.group.access.Unlock()
 	c.group.connections.Remove(c.element)
+	c.group.access.Unlock()
 	return c.PacketConn.Close()
 }
 
@@ -87,8 +87,8 @@ type SingPacketConn struct {
 
 func (c *SingPacketConn) Close() error {
 	c.group.access.Lock()
-	defer c.group.access.Unlock()
 	c.group.connections.Remove(c.element)
+	c.group.access.Unlock()
 	return c.PacketConn.Close()
 }
 

common/interrupt/group.go

@@ -47,15 +47,19 @@ func (g *Group) NewSingPacketConn(conn N.PacketConn, isExternal bool, isProvider
 
 func (g *Group) Interrupt(interruptExternalConnections bool) {
 	g.access.Lock()
-	defer g.access.Unlock()
 	var toDelete []*list.Element[*groupConnItem]
+	var toClose []io.Closer
 	for element := g.connections.Front(); element != nil; element = element.Next() {
 		if !element.Value.isExternal || interruptExternalConnections {
-			element.Value.conn.Close()
 			toDelete = append(toDelete, element)
+			toClose = append(toClose, element.Value.conn)
 		}
 	}
 	for _, element := range toDelete {
 		g.connections.Remove(element)
 	}
+	g.access.Unlock()
+	for _, conn := range toClose {
+		_ = conn.Close()
+	}
 }

common/interrupt/group_test.go

@@ -0,0 +1,66 @@
+package interrupt
+
+import (
+	"net"
+	"sync"
+	"testing"
+	"time"
+)
+
+type closeBarrierConn struct {
+	net.Conn
+	barrier *sync.WaitGroup
+}
+
+func (c *closeBarrierConn) Close() error {
+	c.barrier.Done()
+	c.barrier.Wait()
+	return c.Conn.Close()
+}
+
+func TestNestedGroupsInterruptWithoutDeadlock(t *testing.T) {
+	groupA := NewGroup()
+	groupB := NewGroup()
+	barrier := &sync.WaitGroup{}
+	barrier.Add(2)
+
+	barrierA, barrierAPeer := net.Pipe()
+	barrierB, barrierBPeer := net.Pipe()
+	t.Cleanup(func() {
+		barrierAPeer.Close()
+		barrierBPeer.Close()
+	})
+	groupA.NewConn(&closeBarrierConn{Conn: barrierA, barrier: barrier}, true, false)
+	groupB.NewConn(&closeBarrierConn{Conn: barrierB, barrier: barrier}, true, false)
+
+	connA, connAPeer := net.Pipe()
+	connB, connBPeer := net.Pipe()
+	t.Cleanup(func() {
+		connAPeer.Close()
+		connBPeer.Close()
+	})
+	wrapperA := groupA.NewConn(connA, true, false)
+	wrapperB := groupB.NewConn(connB, true, false)
+	groupA.NewConn(wrapperB, true, false)
+	groupB.NewConn(wrapperA, true, false)
+
+	done := make(chan struct{}, 2)
+	go func() {
+		groupA.Interrupt(true)
+		done <- struct{}{}
+	}()
+	go func() {
+		groupB.Interrupt(true)
+		done <- struct{}{}
+	}()
+
+	timeout := time.NewTimer(time.Second)
+	defer timeout.Stop()
+	for range 2 {
+		select {
+		case <-done:
+		case <-timeout.C:
+			t.Fatal("nested group interrupt deadlocked")
+		}
+	}
+}