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: treat ENFILE as a temporary error, like EMFILE #35131

Open
lmb opened this issue Oct 24, 2019 · 19 comments
Open

syscall: treat ENFILE as a temporary error, like EMFILE #35131

lmb opened this issue Oct 24, 2019 · 19 comments

Comments

@lmb
Copy link
Contributor

@lmb lmb commented Oct 24, 2019

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

Linux

What did you do?

One of our servers ran out of file handles (/proc/sys/fs/file-max). This led net.Listener.Accept() to return ENFILE (not EMFILE). Since ENFILE is not considered a temporary error, http.Server.Serve exited it's accept loop and returned ENFILE.

What did you expect to see?

ENFILE should be treated as temporary if returned by accept (and possible others). http.Server should not abort its accept loop when encountering ENFILE.

What did you see instead?

The application which received ENFILE logged the error, and continued running without a working listener / http server. If the application had followed the common log.Fatalln(http.Serve(...)) it would have crashed.

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 24, 2019

Change https://golang.org/cl/203117 mentions this issue: net: treat ENFILE from accept as a temporary error

@networkimprov
Copy link

@networkimprov networkimprov commented Oct 24, 2019

On Windows, there's WSAEMFILE but according to the page below WSAENOBUFS is more likely, so maybe that should be added on the Windows side?

https://emptyhammock.com/blog/wsaemfile.html

@odeke-em odeke-em changed the title ENFILE from accept is not considered a temporary error net: ENFILE from accept is not considered a temporary error Oct 24, 2019
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Oct 24, 2019

/cc @mikioh per owners.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 25, 2019

If we're going to change this we should change it in the syscall package, in Errno.Temporary, not in the net package.

@ianlancetaylor ianlancetaylor changed the title net: ENFILE from accept is not considered a temporary error syscall: ENFILE from accept is not considered a temporary error Oct 25, 2019
@ianlancetaylor ianlancetaylor changed the title syscall: ENFILE from accept is not considered a temporary error syscall: treat ENFILE as a temporary error, like EMFILE Oct 25, 2019
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 25, 2019

Looks like the check for EMFILE was added in https://golang.org/cl/4550112, but we somehow failed to consider checking for ENFILE at that time. CC @bradfitz who wrote 4550112 in case he remembers anything.

@networkimprov
Copy link

@networkimprov networkimprov commented Oct 25, 2019

Also WSAEMFILE & WSAENOBUFS for syscall_windows.go

cc @alexbrainman @mattn @zx2c4

@lmb
Copy link
Contributor Author

@lmb lmb commented Oct 29, 2019

I've updated the CL to change Errno.Temporary as suggested by @ianlancetaylor. Can someone else tackle the Windows part, or split it into a separate issue? I don't have access to a Windows machine for testing.

@gopherbot gopherbot closed this in 17190de Oct 30, 2019
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 30, 2019

This is fixed on Unix systems, but reopening for Windows.

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 23, 2019

Change https://golang.org/cl/208537 mentions this issue: syscall: treat ENFILE as a temporary error for Windows

@networkimprov
Copy link

@networkimprov networkimprov commented Nov 27, 2019

@alexbrainman @ianlancetaylor the CL for Windows has now eliminated syscall.ENFILE instead of adding WSAEMFILE & WSAENOBUFS to the .Temporary() set. I think we want the latter, and not the former.

Tuning the list of Errno constants for Windows is #32309; a CL for it should consider all the "fake" Errno constants defined in pkg syscall, and decide whether to drop them or set them to a common value.

cc @iWdGo

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 28, 2019

We shouldn't touch any of the fake Errno constants defined for Windows in the syscall package. The gain from that would not be worth the break in backward compatibility.

@networkimprov
Copy link

@networkimprov networkimprov commented Dec 2, 2019

@ianlancetaylor can you endorse adding the WSA* errors I listed to .Temporary()? I believe that was the point of reopening this, but Constantin doesn't believe me :-)

https://emptyhammock.com/blog/wsaemfile.html

@alexbrainman this is the rationale for the CL you asked about.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 2, 2019

Yes, I think that is the appropriate change here.

@networkimprov
Copy link

@networkimprov networkimprov commented Dec 11, 2019

Ian, there is a misunderstanding in the CL thread. Can you reply there, and drop your -2?

https://golang.org/cl/208537

@networkimprov
Copy link

@networkimprov networkimprov commented Jan 15, 2020

@alexbrainman can you explain why WSAEMFILE & WSAENOBUFS are not temporary errors?

This reference shows the latter occurs when server is loaded:
https://emptyhammock.com/blog/wsaemfile.html

@networkimprov
Copy link

@networkimprov networkimprov commented Jan 19, 2020

@alexbrainman sorry, the link I mentioned is posted in this thread, including immediately above. Unfortunately it doesn't give the code he used, but that should be simple to recreate if nec.

@networkimprov
Copy link

@networkimprov networkimprov commented Sep 26, 2020

@djdv this is the only open issue for temporary WSA errors. The original CL author has ceased working on it. If you'd like to take over that work and include the other WSA error you found, that would be great!

See https://golang.org/doc/contribute.html

@djdv
Copy link
Contributor

@djdv djdv commented Sep 29, 2020

@networkimprov
Thanks for that.
I'll put this on my TODO list, but I have some other things to take care of first. If someone wants to beat me to it that'd be fine. :^)
Otherwise I'll look into this soon-ish. (I'll read this thread and linked things in full later)

Specifically, I/someone need(s) to go through the WSA error list and figure out which should be considered "temporary" / what net considers to be "temporary".
Initial assumption being that; as long as the connection can have a successful read in subsequent calls, that it is to be considered temporary. (leaving errors that render the socket unusable as they are now)

@networkimprov
Copy link

@networkimprov networkimprov commented Sep 29, 2020

I think you'd want empirical evidence that an error isn't a cause to drop the connection (and some probably don't occur in practice), so I don't think you'd need to review the WSA list. Let's just add the two discussed above and the one you found.

I think there are only minor scope problems with the posted CL.

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

Successfully merging a pull request may close this issue.

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