Prefer IO#wait_readable/IO#wait_writable rather than IO.select.

[ioquatix]

This allows the Ruby 3 scheduler hooks to improve concurrency of Redis.

Fixes #959

[byroot]

I can't find any documentation on these. In which version were they introduced?

Also it seems like some IOs don't have these methods, like CI shows:

NoMethodError: undefined method `wait_readable' for #<Redis::Connection::SSLSocket:0x0000559ec0a576c0>

[byroot]

Oh nevermind, I found them. It's part of the io/wait extension: http://ruby-doc.com/stdlib-2.3.0_preview1/libdoc/io/wait/rdoc/IO.html

[ioquatix]

Redis::Connection::SSLSocket is not a normal IO so it should be delegating to the OpenSSL::SSL::SSLSocket instance I guess. I'll need to check it.

The method has been around since Ruby 2.0 at least: https://docs.ruby-lang.org/en/2.0.0/IO.html#method-i-wait_readable

[ioquatix]

Hmmmm... It looks like SSLSocket does not expose wait_readable or wait_writable but we can delegate to the underlying I/O.

However, I think we should add this method to OpenSSL gem... I'll coordinate that to happen. It will probably be the same implementation as this PR so there shouldn't be any issue.

[byroot]

I think we should add this method to OpenSSL gem.

Sounds like it indeed.

In the meantime I suppose we can feature check and use IO.select as a fallback?

[ioquatix]

It's very trivial to delegate to the underlying I/O which is completely correct so there should be no need for IO.select.

[ioquatix]

I also noticed this:

          begin
            # Initiate the socket connection in the background. If it doesn't fail
            # immediately it will raise an IO::WaitWritable (Errno::EINPROGRESS)
            # indicating the connection is in progress.
            # Unlike waiting for a tcp socket to connect, you can't time out ssl socket
            # connections during the connect phase properly, because IO.select only partially works.
            # Instead, you have to retry.
            ssl_sock.connect_nonblock
          rescue Errno::EAGAIN, Errno::EWOULDBLOCK, IO::WaitReadable
            if ssl_sock.wait_readable(timeout)
              retry
            else
              raise TimeoutError
            end
          rescue IO::WaitWritable
            if ssl_sock.wait_writable(timeout)
              retry
            else
              raise TimeoutError
            end
          end

I don't have time to investigate WHY this was a problem previously, but I don't know if the retry is needed or not.

[ioquatix]

Do you think you can cut a release after merging this?

[byroot]

I don't have time to investigate WHY

It's late here, and I don't quite remember the details but:

• Apparently I was able to repro 2 years ago: Use nonblocking IO for SSL socket connect. #840
• I took this fix from zendesk/ruby-kafka@83abe89

[ioquatix]

Thanks for the links, if it's a bug with OpenSSL we should definitely investigate further.

[byroot]

Do you think you can cut a release after merging this?

Sure, but I'd like to make sure we didn't regress on the SSL thing first. And I'll probably batch it with a couple other fixes.

[ioquatix]

connect_nonblock(exception: false)

is supported in OpenSSL gem... but not sure how far back it goes (compatibility).

[ioquatix]

Okay, looks like all tests are passing - let me know if you need anything else :)

[byroot]

let me know if you need anything else

Should be good. Also I noticed we actually added a unit test for the SSL hanging issue, so sounds good to me. I'll merge tomorrow morning.