fix: address race condition in socks5 test #278
Conversation
| @@ -177,7 +177,6 @@ func TestConnectWithoutAuth(t *testing.T) { | |||
|
|
|||
| go func() { | |||
| err := server.Serve(listener) | |||
| t.Log("server is listening...") | |||
There was a problem hiding this comment.
What's the race condition? This doesn't seem like it makes any difference.
| @@ -177,7 +177,6 @@ func TestConnectWithoutAuth(t *testing.T) { | |||
|
|
|||
| go func() { | |||
| err := server.Serve(listener) | |||
| t.Log("server is listening...") | |||
| if !errors.Is(err, net.ErrClosed) && err != nil { | |||
| require.NoError(t, err) // Assert no error if it's not the expected close error | |||
| } | |||
There was a problem hiding this comment.
This goroutine must signal when it's done, so that the parent can wait for it. You can use sync.WaitGroup or a channel. Same for TestConnectWithAuth.
|
@fortuna I added sync.WaitGroup to wait for go routines to complete. |
| // Create a SOCKS5 server. | ||
| server := socks5.NewServer() | ||
|
|
||
| // Create SOCKS5 proxy on localhost with a random port. | ||
| listener, err := net.Listen("tcp", "127.0.0.1:0") | ||
| require.NoError(t, err) | ||
| defer listener.Close() | ||
| //defer listener.Close() |
There was a problem hiding this comment.
| //defer listener.Close() |
| @@ -167,17 +167,19 @@ func testExchange(tb testing.TB, listener *net.TCPListener, destAddr string, req | |||
| } | |||
|
|
|||
| func TestConnectWithoutAuth(t *testing.T) { | |||
| var running sync.WaitGroup | |||
There was a problem hiding this comment.
Move this to just before the go func(), so it's clearer you are adding that goroutine
| @@ -192,9 +194,15 @@ func TestConnectWithoutAuth(t *testing.T) { | |||
|
|
|||
| _, err = client.DialStream(context.Background(), address) | |||
| require.NoError(t, err) | |||
| // Forcefully close the listener to stop the server | |||
| // Otherwise defer listener.Close() will never be called | |||
| listener.Close() | |||
There was a problem hiding this comment.
I believe this won't close if the test fails earlier. Instead, defer a function that does both the close and wait.
| } | ||
|
|
||
| func TestConnectWithAuth(t *testing.T) { | ||
| var running sync.WaitGroup |
|
@fortuna thanks for the feedback. I made the changes accordingly. |
|
Thanks for the fix! |
This PR attempts to resolve issue #276