Skip to content
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.

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

transport: don't add connection to multiplexer if init fails #3931

Merged
merged 3 commits into from Jun 29, 2023

Conversation

kelmenhorst
Copy link
Contributor

This diff removes a net.PacketConn from the multiplexer when (*Transport).init fails.

Background: As far as I am aware, a PacketConn is only removed from the multiplexer, when (*Transport).listen returns.
If (*Transport).init returns early, the listen() goroutine is not started, while the PacketConn was already added to the multiplexer, which results in the inability of the Transport to remove the PacketConn entry from the multiplexer.

@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Merging #3931 (dce7555) into master (435444a) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3931      +/-   ##
==========================================
+ Coverage   83.04%   83.15%   +0.11%     
==========================================
  Files         145      145              
  Lines       14595    14594       -1     
==========================================
+ Hits        12120    12135      +15     
+ Misses       1990     1979      -11     
+ Partials      485      480       -5     
Impacted Files Coverage Δ
transport.go 64.21% <100.00%> (+1.34%) ⬆️

... and 4 files with indirect coverage changes

Copy link
Member

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Have you considered calling getMultiplexer().AddConn later, after the code paths that can error (maybe even directly before t.listen)?

@kelmenhorst
Copy link
Contributor Author

Thanks! Yes, I think your suggestion makes sense, I will change it accordingly.

transport_test.go Outdated Show resolved Hide resolved
@marten-seemann marten-seemann changed the title Transport: Remove t.Conn from multiplexer when init fails transport: don't add connection to multiplexer if init fails Jun 29, 2023
@marten-seemann marten-seemann merged commit 0c54d41 into quic-go:master Jun 29, 2023
17 checks passed
bassosimone added a commit to ooni/probe-cli that referenced this pull request Jul 18, 2023
We cannot update quic-go yet because quic-go/quic-go#3931
has not been included into any release, and our tests breaks
without this commit. I am going to write a backport for such a diff
soon, so that we unblock upgrading quic-go.

This work is part of quic-go/quic-go#3931.
bassosimone added a commit to ooni/probe-cli that referenced this pull request Jul 18, 2023
We cannot update quic-go yet because
quic-go/quic-go#3931 has not been included into
any release, and our tests breaks without this commit. I am going to
write a backport for such a diff soon, so that we unblock upgrading
quic-go.

This work is part of ooni/probe#2503.
bassosimone added a commit to ooni/probe-cli that referenced this pull request Jul 18, 2023
This commit backports 538b356.

We cannot update quic-go yet because
quic-go/quic-go#3931 has not been included into
any release, and our tests breaks without this commit. I am going to
write a backport for such a diff soon, so that we unblock upgrading
quic-go.

This work is part of ooni/probe#2503.
Murphy-OrangeMud pushed a commit to Murphy-OrangeMud/probe-cli that referenced this pull request Feb 13, 2024
We cannot update quic-go yet because
quic-go/quic-go#3931 has not been included into
any release, and our tests breaks without this commit. I am going to
write a backport for such a diff soon, so that we unblock upgrading
quic-go.

This work is part of ooni/probe#2503.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants