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

Remove use of deprecated net.Error.Temporary #1589

Merged
merged 1 commit into from
Aug 3, 2024
Merged

Remove use of deprecated net.Error.Temporary #1589

merged 1 commit into from
Aug 3, 2024

Conversation

miekg
Copy link
Owner

@miekg miekg commented Aug 3, 2024

net.Error.Temporary has been deprecated since Go 1.18. There has been some discussion around what to use in server accept loops instead [1], but the suggestion seems to be that it may be a mistake to have any sort of retries in place [2].

This PR removes it, which may expose some users to errors that were previously swallowed and retried, but at the expense of leaking file descriptors and the like.

1: https://groups.google.com/g/golang-nuts/c/-JcZzOkyqYI/m/xwaZzjCgAwAJ?pli=1
2: https://groups.google.com/g/golang-nuts/c/-JcZzOkyqYI/m/vNNiVn_LAwAJ

Thanks for you pull request, do note the following:

  • If your PR introduces backward incompatible changes it will very likely not be merged.

  • We support the last two major Go versions, if your PR uses features from a too new Go version, it
    will not be merged.

net.Error.Temporary has been deprecated since Go 1.18. There
has been some discussion around what to use in server accept loops
instead [1], but the suggestion seems to be that it may be a mistake
to have any sort of retries in place [2].

This PR removes it, which may expose some users to errors that were
previously swallowed and retried, but at the expense of leaking file
descriptors and the like.

1: https://groups.google.com/g/golang-nuts/c/-JcZzOkyqYI/m/xwaZzjCgAwAJ?pli=1
2: https://groups.google.com/g/golang-nuts/c/-JcZzOkyqYI/m/vNNiVn_LAwAJ
@miekg miekg requested a review from tmthrgd as a code owner August 3, 2024 06:05
@miekg miekg mentioned this pull request Aug 3, 2024
@miekg miekg merged commit ef7392e into master Aug 3, 2024
6 checks passed
@miekg miekg deleted the miek/cp1 branch August 3, 2024 06:07
@johanbrandhorst
Copy link
Contributor

I think this change unfortunately interacts badly with the default read timeout used in UDP servers (not TCP servers, in my testing). The default read timeout is 2 seconds, and it seems that any UDP server started via ListenAndServe or ActivateAndServer dies after 2 seconds of no traffic. It seems the TCP server is staying open, not sure why. Presumably this was already happening before, but we were just automatically retrying the error and so it was hidden from us.

Is timing out UDP reads repeatedly while serving intentional? Digging around in the history, it seems the Temporary() check for UDP was introduced to fix a different bug (#621), but I don't see it acknowledged that this will cause us to loop endlessly (albeit not too busily).

Maybe we want to revert this until we have a better understanding of the impact, because on master right now any UDP servers just stop immediately once the ReadTimeout is hit.

@miekg
Copy link
Owner Author

miekg commented Aug 13, 2024

let's revert then. Would be nice if some kind of unit test would have caught this though

miekg added a commit that referenced this pull request Aug 13, 2024
miekg added a commit that referenced this pull request Aug 13, 2024
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.

2 participants