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

syscall: ECONNRESET is not err.Temporary() #24808

Closed
powerman opened this issue Apr 11, 2018 · 12 comments
Closed

syscall: ECONNRESET is not err.Temporary() #24808

powerman opened this issue Apr 11, 2018 · 12 comments

Comments

@powerman
Copy link

@powerman powerman commented Apr 11, 2018

Issue #6163 result in adding ECONNRESET to list of temporary errors. While this may be correct for accept syscall (btw, this errno is not documented on accept(2) and maybe it was returned by accept in some Linux kernel versions by mistake), it's for sure wrong for write/send syscalls: https://stackoverflow.com/a/2979806/113120

Either ECONNRESET should be removed from list of temporary errors, or net.OpError should be modified to count ECONNRESET as Temporary() only when Op=="accept".

@andybons andybons added this to the Unplanned milestone Apr 11, 2018
@andybons
Copy link
Member

@andybons andybons commented Apr 11, 2018

/cc @bradfitz

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 11, 2018

@powerman, are you reporting a regression in Go 1.4?

The stackoverflow thread is about Python.

Is there some context I'm missing about why this is now relevant, years later?

Did this cause a bug somewhere, or were you just reading code?

@powerman
Copy link
Author

@powerman powerman commented Apr 11, 2018

@bradfitz Stackoverflow is about syscalls, python is just an example and should have no importance. Yes, I'm just reading the code and man pages - issue looks obvious, but if it's required I can try to implement example from stackoverflow in Go. Original issue was initiated by question in maillist: https://groups.google.com/forum/#!topic/golang-nuts/5Mcc-idtQcg

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 11, 2018

I'm reluctant to change this because it seems more likely to break more people than it fixes and it's been like this forever.

But if you have a good example of a Go program where the behavior is clearly wrong, we could try it out and see what the fallout looks like.

@powerman
Copy link
Author

@powerman powerman commented Apr 11, 2018

Actually it's enough to output Temporary() at end of example mentioned in maillist to see:

0
1
2
3
4
write tcp 127.0.0.1:43412->127.0.0.1:40447: write: connection reset by peer
temporary: true

Here is modified code: https://play.golang.org/p/Jphm_VSzM1H (run on Linux, not in playground).
Probably it can be simplified even more, but, as I said, issue is obvious: ECONNRESET means TCP RST packet was received, which is temporary error in accept() and fatal in write()/send().

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 11, 2018

Sure, we can try special-casing "accept". @ianlancetaylor?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 11, 2018

Sure.

@gopherbot
Copy link

@gopherbot gopherbot commented May 1, 2018

Change https://golang.org/cl/110515 mentions this issue: net: ECONNRESET is Temporary when accept fails

@gopherbot
Copy link

@gopherbot gopherbot commented May 1, 2018

Change https://golang.org/cl/110439 mentions this issue: net, syscall: make ECONNRESET/ECONNABORTED only temporary for Accept

@networkimprov
Copy link

@networkimprov networkimprov commented Aug 9, 2018

Please add the Go 1.11 milestone to this issue.

I just encountered this in real-world use by reporting net.Error.Temporary() in logs, in hopes of seeing actually-recoverable errors :-)

@ianlancetaylor ianlancetaylor modified the milestones: Unplanned, Go1.11 Aug 9, 2018
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 9, 2018

@networkimprov Done, but in general we don't promise that every issue has the milestone in which it was fixed. It's a good idea but it's not something we have any way of doing reliably.

@dcormier
Copy link
Contributor

@dcormier dcormier commented Jul 2, 2019

I'm reluctant to change this because it seems more likely to break more people than it fixes and it's been like this forever.

Confirmed. Broke something I had in place in a hard-to-notice way. It's probably been broken for months, now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.