Skip IO.try_convert in ruby code for SSL Sockets

[MSP-Greg]

When using SSL sockets, in the c extension code, monitor.io is an SSLSocket, but in Ruby code, monitor.io is a TCPSocket.

[ioquatix]

Just wondering about this - should we really be doing this in the library code? It seems odd to me that we shouldn't just expect the user to supply an IO. @tarcieri what is the logic behind trying to convert it to an IO - in what situations is this useful?

[tarcieri]

IIRC, it was originally to deal with things like SSLSocket which are wrappers/"newtypes" around a real underlying socket object, although in this case it appears to be having the opposite of the intended effect.

[MSP-Greg]

@tarcieri

I kept the scope of the change narrow. I used:

unless io.is_a? OpenSSL::SSL::SSLSocket

Maybe a more general statement?

unless io.is_a? IO or io.is_a? OpenSSL::Buffering

or

unless IO === io or OpenSSL::Buffering === io

[ioquatix]

The way I think this should be solved is to use to_io if necessary. Another option is to assume filedes works, but I'm pretty sure this doesn't work for OpenSSL sockets (yet).

[MSP-Greg]

this doesn't work for OpenSSL sockets (yet).

OpenSSL::Buffering (quoted from doc): 'This module allows an SSL::SSLSocket to behave like an ::IO.'

I believe the above accounts for all the cases in Ruby Core without trying to_io or try_convert...

[ioquatix]

The main issue is really just getting the file descriptor, which is not supported, but for which we could monkey patch.

e.g.

pry(main)> IO.pipe.first.fileno                                                                                                                                                            
=> 18

pry(main)> OpenSSL::SSL::SSLSocket.new(IO.pipe.first).fileno                                                                                                                              
NoMethodError: undefined method `fileno' for #<OpenSSL::SSL::SSLSocket:0x00007fafd9c41b30>

ruby/openssl#198

The correct interface should be fileno and that's what we should use to create the monitor.

[MSP-Greg]

I did the PR to line up the Ruby code monitor.io object with the extension code object. The extension code returned the SSL socket, the Ruby code returned the TCP socket...

[ioquatix]

Yeah, I know it's super confusing, it should obviously be the same.

[ioquatix]

What I'm suggesting is that we remove all this crap and try to have a standard interface. It's not even so much that it's an IO object, it's just that it implements #fileno (although I don't think that works in general, but it's a good first step for consistency).

[MSP-Greg]

Possibly here:
https://www.openssl.org/docs/man1.1.1/man3/BIO_s_fd.html

couldn't find the call anywhere in Ruby OpenSSL...

[MSP-Greg]

BTW, speaking of OpenSSL, etc, Puma now has nio4r as a dependency, and they're using the own OpenSSL implementation...

[ioquatix]

Is there some reason why they use their own OpenSSL implementation?

[MSP-Greg]

Someone asked recently, don't recall if they really got an answer.

It's called 'minissl'; I believe it dates to 2012. Probably "Don't fix what isn't broken" & limited resources...

[ioquatix]

Honestly, my first reaction is that sounds kind of painful.

[ioquatix]

I don't know why we should use IO.try_convert(io) over io.to_io. Maybe try_convert does better error handling. try_convert should invoke to_io so I don't know why it doesn't work for OpenSSL::SSL::SSLSocket.

[MSP-Greg]

Honestly, my first reaction is that sounds kind of painful.

Just to clarify, Puma does not use Ruby OpenSSL, but they are compiling with OpenSSL.

Re try_convert, not available with Ruby OpenSSL or Puma, but both have to_io.

I need to check the Puma tests tomorrow, as I'm not sure if the change to nio4r is being tested with SSL.

Off topic: Speaking of YARD, see https://msp-greg.github.io/index.html. I modified the code to get nio4r to show the c source, but it messes with the repo links...

[ioquatix]

For critical libraries, we can add external tests. If all specs, pass, we checkout external code, modify gem file a little bit to use this code, and then run their tests too. It's an option to ensure we don't break compatibility with upstream. What do you think?

[ioquatix]

@MSP-Greg IO.try_convert should internally invoke to_io.

https://www.rubydoc.info/stdlib/core/IO.try_convert

But it also checks the result is an IO.

https://github.com/ruby/ruby/blob/56557ec28a8712984a0e9744fd7547e797ec9b6b/io.c#L708-L712

https://github.com/ruby/ruby/blob/56557ec28a8712984a0e9744fd7547e797ec9b6b/object.c#L3050-L3064

So I guess it should work as long as the object has to_io that works correctly.