soju: conn: fix goroutine leak v1 APPLIED

Conrad Hoffmann: 1
 conn: fix goroutine leak

 1 files changed, 1 insertions(+), 1 deletions(-)

[PATCH soju] conn: fix goroutine leak Export this patch

As is, soju leaks goroutines on client disconnects, because the closure
started as goroutine in `newConn()` never finishes. It gets stuck in the
for loop annotated as "draining the outgoing channel", because the
outgoing channel is in fact never closed.

This commit fixes the issue by calling `Close()` on `c` rather than
`c.conn`, which closes not only the connection (`c.conn`), but also the
channel.
---
Please note that I am not awefully familiar with the soju code base.
While the proposed fix definitely fixes the leak and I have not observed
any side effects in a test setup, I am not 100% sure about its semantic
correctness. It just seemed very likely that this is what the code
intended to do in the first place.

The issue is pretty easy to reproduce (simply connect a client to soju
and disconnect), but I am happy to supply further data/debug code if so
desired.

 conn.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/conn.go b/conn.go
index 983f976..291363f 100644
--- a/conn.go
+++ b/conn.go
@@ -158,7 +158,7 @@ func newConn(srv *Server, ic ircConn, options *connOptions) *conn {
 				break
 			}
 		}
-		if err := c.conn.Close(); err != nil && !errors.Is(err, net.ErrClosed) {
+		if err := c.Close(); err != nil && !errors.Is(err, net.ErrClosed) {
 			c.logger.Printf("failed to close connection: %v", err)
 		} else {
 			c.logger.Debugf("connection closed")
-- 
2.45.2

builds.sr.ht <builds@sr.ht>

8 months ago

soju/patches/.build.yml: FAILED in 1m7s

[conn: fix goroutine leak][0] from [Conrad Hoffmann][1]

[0]: https://lists.sr.ht/~emersion/soju-dev/patches/53552
[1]: mailto:ch@bitfehler.net

✗ #1262483 FAILED soju/patches/.build.yml https://builds.sr.ht/~emersion/job/1262483

Conrad Hoffmann <ch@bitfehler.net>

8 months ago

Okay, so the behaviour in the test case is what happens when the server 
shuts down while the client is still connected. This gives the following 
matrix, IIUC:

No patch, server shutdown: channel closed
No patch, client shutdown: channel stays open, leak
Patch, server shutdown: double close, panic
Patch, client shutdown: channel closed

I have a hard time wrapping my head around this, especially as the 
closing of the channel is wrapped in a lock and protected against double 
close? But I also haven't fully grokked the usage of ircConn interface 
as a member of conn, as the implementation seems to be another conn? Not 
sure, I'll try to take a closer look tomorrow, but if you have any ideas 
I'd be keen to hear them :)

Cheers,
Conrad