Make file descriptors from stdlib non-inheritable by default #13201
Conversation
alaviss
force-pushed
the
fd-no-leak
branch
2 times, most recently
from
e72420e to
cbf71e0
Compare
|
@alaviss and others: This might be useful - https://www.python.org/dev/peps/pep-0446/ |
|
I'm not a fan of fixing UNIX bugs in Nim, but I won't fight it. |
Windows leak handles too, unless you manually use Win32 API. |
I always wanted to use the Win32 API directly for IO indeed. |
alaviss
force-pushed
the
fd-no-leak
branch
2 times, most recently
from
91aa08f to
741130e
Compare
|
But isn't this a heavy-handed change for the language to impose on the user? How does the user get around it? |
By marking file descriptors that you want to pass down with And I don't even think most users will ever notice these changes. Maybe they will find out that their web server no longer error out due to "address in use" when it restarts (see #6602), but for the few that actually use this functionality, chances are that they know how to do it correctly. |
|
That makes sense; thanks for explaining it. |
|
That should be about all of them. Please let me know if I missed any. |
|
Reopening for Windows CI which failed for unrelated reasons. |
|
It's unfortunate there's no way to enable a test at compile time to list or detect inherited file descriptors |
lib/system/io.nim
Outdated
| var p = fopen(filename, FormatOpen[mode]) | ||
| ## | ||
| ## The file handle associated with the resulting ``File`` is not inheritable. | ||
| var p = fopen(filename, FormatOpen[mode] & NoInheritFlag) |
There was a problem hiding this comment.
And then use no string concats here.
Ha, good one. I don't know either. |
|
There has been concerns about how this makes multi-process servers much harder to write than before. Can we add a build option like |
File descriptors from ioselectors should no longer leaks on Linux.
Also simplified the test by using handle-type agnostic APIs to test for validity.
The getFd proc for ioselectors_select returns a hardcoded -1
This allows for the flag to be set/unset in one syscall. While the performance gains might be negliable, we have one less failure point to deal with.
On Windows, the native handle is the only thing that's inheritable, thus we can assume that users of this function will already have the handle available to them. This also allows users to pass down file descriptors from memfiles on Windows with ease, should that be desired. With this, nativesockets.setInheritable can be made much simpler.
| @@ -81,7 +81,7 @@ proc newSelector*[T](): Selector[T] = | |||
| # Start with a reasonable size, checkFd() will grow this on demand | |||
| const numFD = 1024 | |||
|
|
|||
| var epollFD = epoll_create(MAX_EPOLL_EVENTS) | |||
| var epollFD = epoll_create1(O_CLOEXEC) | |||
There was a problem hiding this comment.
epoll_create1 was added to the Linux in version 2.6.27. Is it ok for Nim's minimal supported Linux version?
http://man7.org/linux/man-pages/man2/epoll_create.2.html
There was a problem hiding this comment.
It's released over a decade ago, I don't see why not :)
There was a problem hiding this comment.
Hmm, but isn't Linux 2.4 still widely used for embedded development?
There was a problem hiding this comment.
We don't have to care about that, since ioselectors_epoll uses eventfd(2) which is added in Linux 2.6.22.
|
I find one more case: |
|
@Araq It's ready for review. |
Fixes #6602. Please refer to the linked issue for the rationale.
This is still pretty much a WIP. The final goal here is to make all file descriptors created from stdlib's high-level abstractions non-inheritable by default.