-
-
Notifications
You must be signed in to change notification settings - Fork 921
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
undefined "close_on_exec=" for TCPSocket #4858
Comments
The default behavior for all IO streams on JRuby is to not close on exec. MRI defaults the other way. The main complexity here is that we don't have a way to implement We can certainly open up the The other methods are simple enough to add. |
FWIW, MRI does not implement these methods on all platforms; only on those which support CLOEXEC. I think that would exclude Windows at least, but not sure about others. |
That's actually good to know, that's my intended case. |
and thx for the fix :) |
@headius I added a script below which doesn't really show a tcp socket being preserved after exec. This is a small reproduction of the larger problem that I have: passing listener fds in the env, exec and keep the reference. require "socket"
server = TCPServer.new("localhost", 9999)
if ENV.key?("SOCKID")
fd = ENV["SOCKID"].to_i
puts "we have a winner: #{fd}"
s = TCPServer.for_fd(fd)
puts "server: #{s.inspect}"
else
ENV["SOCKID"] = server.fileno.to_s
exec($0)
end
#######
# we have a winner: 20
# IOError: closed stream |
Hmm...perhaps I was wrong about the JDK not setting CLOEXEC. I don't believe it used to, but they may have added it. Grepping the jdk sources, it looks like the JDK will set all of the following cloexec:
So the issue of wrapping the JDK socket classes becomes more of a problem. If they can't be unwrapped to a file descriptor, and we can't change their cloexec flag, they'll always go away. In JRuby, the following streams will usually be "truly native" and allow you to unset CLOEXEC: (This applies to UNIX environments only)
Because of the depth of work required to support the entire socket library, we delegate most of that to the JDK, which leads to our current predicament. |
Reopen; the issue is fixed for streams that are backed by a native file descriptor, but TCPSocket and most of the other socket classes are not native and will not respond to close_on_exec=. |
I've repeated the script using the |
CLOEXEC can be implemented on a piecemeal basis for the streams which are native, as listed above. Since we can't fix it for all cases all the time, perhaps you can illustrate which cases are important to you for this issue? |
Adds UNIXServer/Socket support for #4858.
With 12bb2fa, we should now support CLOEXEC for all streams which we back with a "real" file descriptor, which includes most items I list in #4858 (comment). At this time I do not believe IO.pipe produces native streams, but it could. It may have to move to FFI on Java 9 anyway due to the impossibility of digging out the real file descriptors. Currently, if we can dig it out reflectively, it will work with CLOEXEC. It will not otherwise. Tempfile is the other notable exception, along with all IO on Windows or platforms where we don't have FFI support. If we can't dig the file descriptor out reflectively, CLOEXEC will fail to work. |
* Port 2015 redirect logic for popen. * Improve cloexec for subprocess IO. * Make cloexec work for any streams we can get realFileno for. ** This allows it to work for JVM-provided pipes if they can be unwrapped. See #4858
Adds UNIXServer/Socket support for #4858.
* Port 2015 redirect logic for popen. * Improve cloexec for subprocess IO. * Make cloexec work for any streams we can get realFileno for. ** This allows it to work for JVM-provided pipes if they can be unwrapped. See #4858
I'm going to call the original issue here fixed, since I'll file a separate issue about native Pipe and cloexec. |
FYI, at least one MRI-provided cloexec test passes, after I spent a lot of time fiddling with cloexec logic. So the testing side of this is largely covered. |
I finally had time to look on the issue, and I saw that 9.1.15.0 was released, thx! Testing my script, I see that the unix sockets now live beyond exec, and that's cool! I tried it again with tcp sockets, as I wasn't sure whether that was addressed by your fixes, and still doesn't work, but I think that you said that was a limitation of the JDK sockets and access to the real fd. What confused me was your statement:
This is still a TODO, right? As this should have warned me that this operation is impossible with tcp sockets, and it didn't. |
This seems to have broken selenium-webdriver (3.7.0) and childprocess (0.8.0). This combination raises an exception (on windows) —
|
@HoneyryderChuck Can you give me a simple example that shows TCPSocket is not staying alive into the child? @ketan Ugh, ok. Windows has almost no native IO, so it should be just warning or no-op'ing the cloexec call. Can you open a separate issue for that please, and we'll fix it for 9.1.16.0? |
Reopen while we investigate lingering issues with TCPSocket. |
@headius just running the script I pasted previously. |
@HoneyryderChuck Ok, what I can see is the following:
So the real issues that remain are that setting CLOEXEC on a JDK socket fd does not appear to change its lifecycle after exec, and that JDK-originated streams can't be "for_fd" in a subprocess, because we have no way to reconstitute the JDK-provided object that wraps them. |
Environment
I'm getting the following error when using the "close_on_exec" APIs in TCP Server/Sockets:
At first I just thought this was one of the MRI/JRuby differences, but then I saw there are tests implemented for this in jruby, and the "did you mean" functionality throws out some weird errors when I'm pretty close to the method name:
From what I understood, this should be implemented, and it's important for the "exec and keep IOs open in child process" functionality, which jruby should support.
The text was updated successfully, but these errors were encountered: