magicsock: if STUN failed to send before, rebind before STUNning again. #3014
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
apenwarr
force-pushed
the
apenwarr/stunbind
branch
3 times, most recently
from
cfd952f
to
d530e6d
Compare
crawshaw
approved these changes
Oct 7, 2021
bradfitz
approved these changes
Oct 7, 2021
net/netcheck/netcheck.go
Outdated
| IPv6 bool // IPv6 works (send & receive) | ||
| IPv6CanSend bool // IPv6 works (send) | ||
| IPv4 bool // IPv4 works (send & receive) | ||
| IPv4CanSend bool // IPv4 works (send) |
There was a problem hiding this comment.
clarify whether these new CanSend fields are any vs all?
There was a problem hiding this comment.
Technically all four of them are "any" and not "all".
There was a problem hiding this comment.
updated for more verbosity.
On iOS (and possibly other platforms), sometimes our UDP socket would get stuck in a state where it was bound to an invalid interface (or no interface) after a network reconfiguration. We can detect this by actually checking the error codes from sending our STUN packets. If we completely fail to send any STUN packets, we know something is very broken. So on the next STUN attempt, let's rebind the UDP socket to try to correct any problems. This fixes a problem where iOS would sometimes get stuck using DERP instead of direct connections until the backend was restarted. Fixes #2994 Signed-off-by: Avery Pennarun <apenwarr@tailscale.com>
apenwarr
force-pushed
the
apenwarr/stunbind
branch
from
d530e6d
to
70b9501
Compare
DentonGentry
added a commit
that referenced
this pull request
Oct 16, 2021
#3014 added a rebind on STUN failure, which means there can now be a call in tailscale.com/wgengine/magicsock.(*RebindingUDPConn).ReadFromNetaddr at the end of the test, waiting for a STUN response which will never arrive. The WG scaffolding to call bind.Close() is not present in this these tests, so ensure it gets called explicitly. Signed-off-by: Denton Gentry <dgentry@tailscale.com>
DentonGentry
added a commit
that referenced
this pull request
Oct 16, 2021
#3014 added a rebind on STUN failure, which means there can now be a tailscale.com/wgengine/magicsock.(*RebindingUDPConn).ReadFromNetaddr in progress at the end of the test, waiting for a STUN response which will never arrive. This causes a test failure due to the resource leak. For whatever reason, it mostly fails with Windows. The WG scaffolding to call bind.Close() is not present in this these tests, so ensure it gets called explicitly. Signed-off-by: Denton Gentry <dgentry@tailscale.com>
DentonGentry
added a commit
that referenced
this pull request
Oct 16, 2021
#3014 added a rebind on STUN failure, which means there can now be a tailscale.com/wgengine/magicsock.(*RebindingUDPConn).ReadFromNetaddr in progress at the end of the test, waiting for a STUN response which will never arrive. This causes a test failure due to the resource leak. For whatever reason, it mostly fails with Windows. The WG scaffolding to call bind.Close() is not present in this these tests, so ensure it gets called explicitly. Signed-off-by: Denton Gentry <dgentry@tailscale.com>
DentonGentry
added a commit
that referenced
this pull request
Oct 16, 2021
#3014 added a rebind on STUN failure, which means there can now be a tailscale.com/wgengine/magicsock.(*RebindingUDPConn).ReadFromNetaddr in progress at the end of the test, waiting for a STUN response which will never arrive. This causes a test failure due to the resource leak. For whatever reason, it mostly fails with Windows. The WG scaffolding to call bind.Close() is not present in this these tests, so ensure it gets called explicitly. Signed-off-by: Denton Gentry <dgentry@tailscale.com>
DentonGentry
added a commit
that referenced
this pull request
Oct 16, 2021
#3014 added a rebind on STUN failure, which means there can now be a tailscale.com/wgengine/magicsock.(*RebindingUDPConn).ReadFromNetaddr in progress at the end of the test, waiting for a STUN response which will never arrive. This causes a test failure due to the resource leak. For whatever reason, it mostly fails with Windows. The WG scaffolding to call bind.Close() is not present in this these tests, so ensure it gets called explicitly. Signed-off-by: Denton Gentry <dgentry@tailscale.com>
DentonGentry
added a commit
that referenced
this pull request
Oct 16, 2021
#3014 added a rebind on STUN failure, which means there can now be a tailscale.com/wgengine/magicsock.(*RebindingUDPConn).ReadFromNetaddr in progress at the end of the test, waiting for a STUN response which will never arrive. This causes a test failure due to the resource leak. For whatever reason, it mostly fails with Windows. The WG scaffolding to call bind.Close() is not present in this these tests, so ensure it gets called explicitly. Signed-off-by: Denton Gentry <dgentry@tailscale.com>
DentonGentry
added a commit
that referenced
this pull request
Oct 16, 2021
#3014 added a rebind on STUN failure, which means there can now be a tailscale.com/wgengine/magicsock.(*RebindingUDPConn).ReadFromNetaddr in progress at the end of the test, waiting for a STUN response which will never arrive. This causes a test failure due to the resource leak. For whatever reason, it mostly fails with Windows. The WG scaffolding to call bind.Close() is not present in this these tests, so ensure it gets called explicitly. Signed-off-by: Denton Gentry <dgentry@tailscale.com>
DentonGentry
added a commit
that referenced
this pull request
Oct 16, 2021
#3014 added a rebind on STUN failure, which means there can now be a tailscale.com/wgengine/magicsock.(*RebindingUDPConn).ReadFromNetaddr in progress at the end of the test, waiting for a STUN response which will never arrive. This causes a test failure due to the resource leak. For whatever reason, it mostly fails with Windows. The WG scaffolding to call bind.Close() is not present in this these tests, so ensure it gets called explicitly. Signed-off-by: Denton Gentry <dgentry@tailscale.com>
DentonGentry
added a commit
that referenced
this pull request
Oct 16, 2021
#3014 added a rebind on STUN failure, which means there can now be a tailscale.com/wgengine/magicsock.(*RebindingUDPConn).ReadFromNetaddr in progress at the end of the test waiting for a STUN response which will never arrive. This causes a test flake due to the resource leak in those cases where the Conn decided to rebind. For whatever reason, it mostly flakes with Windows. If the Conn is closed, don't Rebind after a send error. Signed-off-by: Denton Gentry <dgentry@tailscale.com>
DentonGentry
added a commit
that referenced
this pull request
Oct 16, 2021
#3014 added a rebind on STUN failure, which means there can now be a tailscale.com/wgengine/magicsock.(*RebindingUDPConn).ReadFromNetaddr in progress at the end of the test waiting for a STUN response which will never arrive. This causes a test flake due to the resource leak in those cases where the Conn decided to rebind. For whatever reason, it mostly flakes with Windows. If the Conn is closed, don't Rebind after a send error. Signed-off-by: Denton Gentry <dgentry@tailscale.com>
DentonGentry
added a commit
that referenced
this pull request
Oct 17, 2021
#3014 added a rebind on STUN failure, which means there can now be a tailscale.com/wgengine/magicsock.(*RebindingUDPConn).ReadFromNetaddr in progress at the end of the test waiting for a STUN response which will never arrive. This causes a test flake due to the resource leak in those cases where the Conn decided to rebind. For whatever reason, it mostly flakes with Windows. If the Conn is closed, don't Rebind after a send error. Signed-off-by: Denton Gentry <dgentry@tailscale.com>
bradfitz
pushed a commit
that referenced
this pull request
Oct 19, 2021
#3014 added a rebind on STUN failure, which means there can now be a tailscale.com/wgengine/magicsock.(*RebindingUDPConn).ReadFromNetaddr in progress at the end of the test waiting for a STUN response which will never arrive. This causes a test flake due to the resource leak in those cases where the Conn decided to rebind. For whatever reason, it mostly flakes with Windows. If the Conn is closed, don't Rebind after a send error. Signed-off-by: Denton Gentry <dgentry@tailscale.com> (cherry picked from commit def650b)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On iOS (and possibly other platforms), sometimes our UDP socket would
get stuck in a state where it was bound to an invalid interface (or no
interface) after a network reconfiguration. We can detect this by
actually checking the error codes from sending our STUN packets.
If we completely fail to send any STUN packets, we know something is
very broken. So on the next STUN attempt, let's rebind the UDP socket
to try to correct any problems.
This fixes a problem where iOS would sometimes get stuck using DERP
instead of direct connections until the backend was restarted.
Fixes #2994
Signed-off-by: Avery Pennarun apenwarr@tailscale.com