Skip to content

Ensure Close() waits for goroutines when called concurrently #331

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+21 −30
Open

Ensure Close() waits for goroutines when called concurrently #331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 20 additions & 23 deletions net/multi_listen.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"fmt"
"net"
"sync"
"sync/atomic"
)

// connErrPair pairs conn and error which is returned by accept on sub-listeners.
Expand All @@ -38,8 +37,8 @@ type multiListener struct {
// connCh passes accepted connections, from child listeners to parent.
connCh chan connErrPair
// stopCh communicates from parent to child listeners.
stopCh chan struct{}
closed atomic.Bool
stopCh chan struct{}
closeOnce sync.Once
}

// compile time check to ensure *multiListener implements net.Listener
Expand Down Expand Up @@ -152,29 +151,27 @@ func (ml *multiListener) Accept() (net.Conn, error) {
// the go-routines to exit.
func (ml *multiListener) Close() error {
// Make sure this can be called repeatedly without explosions.
if !ml.closed.CompareAndSwap(false, true) {
return fmt.Errorf("use of closed network connection")
}

// Tell all sub-listeners to stop.
close(ml.stopCh)

// Closing the listeners causes Accept() to immediately return an error in
// the sub-listener go-routines.
for _, l := range ml.listeners {
_ = l.Close()
}
ml.closeOnce.Do(func() {
// Tell all sub-listeners to stop.
close(ml.stopCh)

// Closing the listeners causes Accept() to immediately return an error in
// the sub-listener go-routines.
for _, l := range ml.listeners {
_ = l.Close()
}

// Wait for all the sub-listener go-routines to exit.
ml.wg.Wait()
close(ml.connCh)
// Wait for all the sub-listener go-routines to exit.
ml.wg.Wait()
close(ml.connCh)

// Drain any already-queued connections.
for connErr := range ml.connCh {
if connErr.conn != nil {
_ = connErr.conn.Close()
// Drain any already-queued connections.
for connErr := range ml.connCh {
if connErr.conn != nil {
_ = connErr.conn.Close()
}
Copy link
Member Author

@ash2k ash2k Aug 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why this loop is necessary. Channel is not buffered and we've told the publishing goroutines to stop just above via close(ml.stopCh) - they will each close the connection they are trying to push, if any.

}
}
})
return nil
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it doesn't make sense to return an error for double close. Either we return the actual underlying errors or no error at all. Any reason to create "artificial" errors? I.e. nothing bad happened because of the second call, why return an error?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative implementation here would still synchronize the shutdown of goroutines, but call Close on listeners as many times as this Close is called and then return the real errors, if any. I.e. pass the duplicate call through.

}

Expand Down
8 changes: 1 addition & 7 deletions net/multi_listen_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,6 @@ func TestMultiListen_Close(t *testing.T) {
runner func(listener net.Listener, acceptCalls int) error
fakeListeners []*fakeListener
acceptCalls int
errString string
}{
{
name: "close",
Expand Down Expand Up @@ -327,7 +326,6 @@ func TestMultiListen_Close(t *testing.T) {
return nil
},
fakeListeners: []*fakeListener{{}, {}, {}},
errString: "use of closed network connection",
},
}

Expand All @@ -339,11 +337,7 @@ func TestMultiListen_Close(t *testing.T) {
t.Errorf("Did not expect error: %v", err)
}
err = tc.runner(ml, tc.acceptCalls)
if tc.errString != "" {
assertError(t, tc.errString, err)
} else {
assertNoError(t, err)
}
assertNoError(t, err)

for _, f := range tc.fakeListeners {
if !f.closed.Load() {
Expand Down
Loading