fix: Close conn in quic listener when wrapping fails #2852
Conversation
p2p/transport/quic/listener_test.go
Outdated
| } | ||
|
|
||
| // No error yet, let's continue using the conn | ||
| s.SetReadDeadline(time.Now().Add(1 * time.Second)) |
There was a problem hiding this comment.
We can keep this 10 seconds. It will error before that any way.
| _, err := client.NewStream(ctx, server.ID(), ping.ID) | ||
| require.Error(t, err) | ||
| require.False(t, errors.Is(err, context.DeadlineExceeded), "expected error to be not be context deadline exceeded") |
There was a problem hiding this comment.
This will succeed if the implementation is using lazy multistream negotiation, right?
There was a problem hiding this comment.
In which case the require.Error will fail. I haven't seen that happen which is probably because we require the identify to happen before returing a new stream (a separate issue). For now I think let's leave this as is and when we change the Identify/NewStream interaction we can update this test.
| if tc.Name == "WebRTC" { | ||
| continue // WebRTC doesn't have a connection when we block so there's nothing to close |
There was a problem hiding this comment.
Should we test this, NewStream on client will still fail, which is what we want. It'll fail with DeadlineExceeded though
There was a problem hiding this comment.
I don't think we should. It's not testing what we think. The only thing it tests is whether the server ignores us and we hit a deadline.
There was a problem hiding this comment.
Maybe we could make another tests that just focuses on that behavior for WebRTC though?
There was a problem hiding this comment.
It would test that if the respurce manager blocks no connection is established. But we can handle that separately. I am fine with keeping it as is.
|
Did you merge this instead of squash and merge? 😅 |
|
Unfortunately yes. Not on purpose. I feel like the defaults might have changed. |
|
When will this PR be included in a new release ? |
When wrapping a quic connection into a transport.CapableConn we wouldn't always close the underlying connection if wrapping failed. This fixes that and adds some tests to ensure all transports do this properly.
I believe this closes #2841