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

Socket#close raises EBADF after unsuccessful connection #5709

Open
p0deje opened this issue Apr 25, 2019 · 8 comments

Comments

Projects
None yet
2 participants
@p0deje
Copy link

commented Apr 25, 2019

Environment

  • JRuby version (jruby -v) and command line (flags, JRUBY_OPTS, etc):
jruby 9.2.7.0 (2.5.3) 2019-04-09 8a269e3 Java HotSpot(TM) 64-Bit Server VM 25.172-b11 on 1.8.0_172-b11 +jit [darwin-x86_64]
  • Operating system and platform (e.g. uname -a)
Darwin MacBoosha.local 18.2.0 Darwin Kernel Version 18.2.0: Thu Dec 20 20:46:53 PST 2018; root:xnu-4903.241.1~1/RELEASE_X86_64 x86_64

Expected Behavior

I'm trying to check if the port 9250 is open and as a part of this exercise I execute the following code:

require 'socket'

sock = Socket.new(Socket::AF_INET, Socket::SOCK_STREAM)
addr = Socket.pack_sockaddr_in(9250, '127.0.0.1')

begin
  sock.connect_nonblock(addr)
rescue Errno::EINPROGRESS
  IO.select(nil, [sock], nil, 1) # select socket for writing
  sock.getsockopt(Socket::SOL_SOCKET, Socket::SO_ERROR) # read any errors from last connection
  retry
rescue Errno::ECONNREFUSED, Errno::EINVAL
  puts 'Open!'
end

sock.close

Running this code on MRI (ruby 2.5.5p157 (2019-03-15 revision 67260) [x86_64-darwin18]) raises no error:

$ ruby socket.rb
Open!

Actual Behavior

Running the code on JRuby raises Errno::EBADF when it's trying to close socket:

$ ruby socket.rb
Open!
Errno::EBADF: Bad file descriptor - No message available
   close at org/jruby/ext/socket/RubySocket.java:694
  <main> at socket.rb:16

@p0deje p0deje changed the title Socket#close raises EBADF after connection Socket#close raises EBADF after unsuccesful connection Apr 25, 2019

@p0deje p0deje changed the title Socket#close raises EBADF after unsuccesful connection Socket#close raises EBADF after unsuccessful connection Apr 25, 2019

@headius

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Hmm...our behavior seems more correct, to be honest. In your case the socket is not open (connection refused), so I'd expect close to error.

sock.close rescue nil is a simple workaround for the moment.

@headius

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Yeah as usual I'm still missing some detail about when CRuby will raise this error and when it won't.

It doesn't appear to raise for repeat close on a Socket, pipe, or File:

[] ~/projects/ruby $ rvm ruby-2.5.3 do ruby -rsocket -e 's = TCPSocket.new("google.com", 80); 5.times { s.close }'

[] ~/projects/ruby $ rvm ruby-2.5.3 do ruby -rsocket -e 'r, w = IO.pipe; 5.times { r.close }'

[] ~/projects/ruby $ rvm ruby-2.5.3 do ruby -rsocket -e 'f = File.open("Makefile"); 5.times { f.close }'

[] ~/projects/ruby $ 

However if you open a file descriptor with File.open and close the original descriptor's IO while inside that block, exiting the block triggers EBADF:

$ rvm ruby-2.5.3 do ruby -e 'r, w = IO.pipe; File.open(r.fileno) {|f| r.close}'
Traceback (most recent call last):
	2: from -e:1:in `<main>'
	1: from -e:1:in `open'
-e:1:in `close': Bad file descriptor (Errno::EBADF)

The following patch allows repeated close calls to succeed without error, which fixes your case and potentially others. However the above behavior no longer works correctly. I'm not sure whether it's important or not, but it's in ruby/spec.

diff --git a/core/src/main/java/org/jruby/RubyIO.java b/core/src/main/java/org/jruby/RubyIO.java
index 9552af51c3..c2450b0fd2 100644
--- a/core/src/main/java/org/jruby/RubyIO.java
+++ b/core/src/main/java/org/jruby/RubyIO.java
@@ -1972,7 +1972,8 @@ public class RubyIO extends RubyObject implements IOEncodable, Closeable, Flusha
 
         fptr = openFile;
         checkInitialized();
-        return fptr.fd() == null;
+        ChannelFD fd = fptr.fd();
+        return fd == null || !fd.ch.isOpen();
     }
 
     /**
diff --git a/core/src/main/java/org/jruby/ext/socket/RubySocket.java b/core/src/main/java/org/jruby/ext/socket/RubySocket.java
index 23ae0041b8..757ca11996 100644
--- a/core/src/main/java/org/jruby/ext/socket/RubySocket.java
+++ b/core/src/main/java/org/jruby/ext/socket/RubySocket.java
@@ -685,17 +685,6 @@ public class RubySocket extends RubyBasicSocket {
         return new Addrinfo(runtime, runtime.getClass("Addrinfo"), addr.getAddress(), addr.getPort(), Sock.SOCK_DGRAM);
     }
 
-    @Override
-    @JRubyMethod
-    public IRubyObject close(final ThreadContext context) {
-        if (getOpenFile() != null) {
-            if (isClosed()) return context.nil;
-            openFile.checkClosed();
-            return rbIoClose(context);
-        }
-        return context.nil;
-    }
-
     @Override
     public RubyBoolean closed_p(ThreadContext context) {
         if (getOpenFile() == null) return context.fals;

headius added a commit to headius/jruby that referenced this issue Apr 25, 2019

Allow repeated close of any IO to succeed.
This relates to jruby#5709 but breaks at least one spec:

```
File.open opens a file with a file descriptor d and a block FAILED
Expected Errno::EBADF but no exception was raised (nil was returned)
/Users/headius/projects/jruby/spec/ruby/core/file/open_spec.rb:173:in `block in <main>'
```

There may be other failures, but it's debateable whether they
represent behavior we need to emulate.
@headius

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Pushed a PR to see how many other failures result from the above patch: #5713

@p0deje

This comment has been minimized.

Copy link
Author

commented Apr 26, 2019

In your case the socket is not open (connection refused), so I'd expect close to error.

I'm not good with sockets, but I am the one who opens socket for writing and, even though it can't be written to (connection refused), I am the one responsible for closing it, ain't I?

@headius

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

@p0deje I guess that's a matter of perspective. It wasn't able to connect, so is it actually open? Is a socket with no endpoint open?

@headius

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

Oh a bit of background here: our Socket class is not based on raw native socket calls at the C level. It could be, but that's a lot of work to migrate over and it would force us to always require native support. Instead, we wrap the JVM's socket APIs and simulate the create, bind, accept/connect sequence even though the JVM does all three at once for us under the covers. This means we're not directly in control of the shutdown sequence, so by the time you get to close the JVM socket has already been terminated (since it couldn't connect anyway).

You may have better results with e.g. TCPSocket, but I'm not sure. The same situation applies there...we're leaning on the JDK classes. In this case, the underlying socket has basically been closed already for us, so attempting to close it again raises an error.

@p0deje

This comment has been minimized.

Copy link
Author

commented Apr 27, 2019

I'm actually working this around by using TCPSocket already: https://github.com/SeleniumHQ/selenium/blob/master/rb/lib/selenium/webdriver/common/socket_poller.rb#L68-L77. I'm not sure what can potentially break on other platforms (e.g. WSL on Windows, Cygwin, etc.) if we stick to plain TCPSocket in all the cases.

Moving on, I can definitely keep the workaround, I'm just wondering if this should be anyhow addressed in JRuby.

@headius

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

The long term workaround is probably to make Socket actually use native Socket APIs, but that's a big change. It also doesn't integrate well with the rest of IO on JDK...so we have resisted making that move. Socket in Ruby is just such a low-level wrapper around native Socket APIs, it's hard to emulate without actually calling those same APIs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.