Skip to content

Remove use of deprecated net.Error.Temporary#1589

Merged
miekg merged 1 commit intomasterfrom
miek/cp1
Aug 3, 2024
Merged

Remove use of deprecated net.Error.Temporary#1589
miekg merged 1 commit intomasterfrom
miek/cp1

Conversation

@miekg
Copy link
Copy Markdown
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
@miekg miekg deleted the miek/cp1 branch August 3, 2024 06:07
@johanbrandhorst
Copy link
Copy Markdown
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
Copy Markdown
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

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