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

Only retry UDP socket bind if it returned EADDRINUSE #1761

Merged
merged 1 commit into from
Aug 8, 2022

Conversation

peterthejohnston
Copy link
Contributor

Currently, the NextRandomUdpSocket picks a random port in the ephemeral
port range and attempts to bind a UDP socket to it. If that bind
operation fails, it indiscriminately retries several times before
allowing the caller to poll again, assuming that it failed because the
randomly selected port was in use.

However, this means that if bind fails for another reason than the
requested port being unavailable, such as insufficient capabilities to
bind a socket, this error will not be propagated to the caller. Instead,
with this change, NextRandomUdpSocket immediately returns any error
returned from bind unless it is EADDRINUSE, in which case, it picks
another random port and tries again as before.

This was observed in a test on Fuchsia because, if the DNS resolver does not
have access to the capability required from the network stack to create and bind
sockets, it will see an EPIPE (broken pipe) error on bind.

Currently, the NextRandomUdpSocket picks a random port in the ephemeral
port range and attempts to bind a UDP socket to it. If that bind
operation fails, it indiscriminately retries several times before
allowing the caller to poll again, assuming that it failed because the
randomly selected port was in use.

However, this means that if `bind` fails for another reason than the
requested port being unavailable, such as insufficient capabilities to
bind a socket, this error will not be propagated to the caller. Instead,
with this change, NextRandomUdpSocket immediately returns any error
returned from `bind` unless it is EADDRINUSE, in which case, it picks
another random port and tries again as before.
Copy link
Collaborator

@djc djc left a comment

Choose a reason for hiding this comment

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

Nice improvement!

Copy link
Member

@bluejekyll bluejekyll left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this PR, it should definitely improve things.

@bluejekyll bluejekyll merged commit e166a23 into hickory-dns:main Aug 8, 2022
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

3 participants