Skip to content

Expose IO#timeout and IO#timeout= if available #693

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

Closed
wants to merge 1 commit into from

Conversation

casperisfine
Copy link

@casperisfine
Copy link
Author

Actually, might not be that simple, as the buffering layer breaks on a timeout:

RedisClient::SSLConnectionTest#test_tcp_downstream_latency:
TypeError: no implicit conversion of Integer into Hash
    /opt/rubies/3.2.2/lib/ruby/3.2.0/openssl/buffering.rb:247:in `>='
    /opt/rubies/3.2.2/lib/ruby/3.2.0/openssl/buffering.rb:247:in `gets'

I'll have to dig deeper.

@casperisfine
Copy link
Author

Yeah, definitely doesn't work, OpenSSL buffering uses ossl_ssl_read_internal, which isn't aware of IO#timeout it seems.

@ioquatix
Copy link
Member

ioquatix commented Nov 7, 2023

rb_io_maybe_wait_readable(errno, io, RUBY_IO_TIMEOUT_DEFAULT);

Maybe we need to set it to nonblock mode? In theory, it should use the default timeout.

@casperisfine
Copy link
Author

Maybe we need to set it to nonblock mode?

Perhaps? I'm not quite familiar enough with that code.

@ioquatix
Copy link
Member

ioquatix commented Nov 7, 2023

@rhenium is there a chance you can help look into this? I'm happy to help.

@rhenium
Copy link
Member

rhenium commented Nov 10, 2023

There seem to be two issues before the buffering layer.

IO::Timeout isn't raised by OpenSSL::SSL at all. rb_io_maybe_wait_readable() and rb_io_maybe_wait_writable() on timeout return 0. This return value is currently discarded.

Also, a single blocking operation on TLS, such as #connect and #sysread, can involve multiple blocking operations on the underlying socket. This would mean we can't rely on RUBY_IO_TIMEOUT_DEFAULT, but have to get rb_io_timeout() and subtract elapsed time before waiting again.

@rhenium
Copy link
Member

rhenium commented Nov 10, 2023

rb_io_maybe_wait_readable(errno, io, RUBY_IO_TIMEOUT_DEFAULT);

Maybe we need to set it to nonblock mode? In theory, it should use the default timeout.

The underlying socket is set to non-blocking mode by SSLSocket#initialize. We needed this for implementing SSLSocket#read_nonblock and also because we didn't want blocking IO within libssl while holding the GVL.

@io.timeout = timeout
end
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SocketForwarder (in this file, above) is the right place for this definition.

@ioquatix
Copy link
Member

Also, a single blocking operation on TLS, such as #connect and #sysread, can involve multiple blocking operations on the underlying socket. This would mean we can't rely on RUBY_IO_TIMEOUT_DEFAULT, but have to get rb_io_timeout() and subtract elapsed time before waiting again.

Thanks for this clarification. IO#timeout was designed with discrete, low level operations in mind. I think we can start with the simplest implementation, which is discrete timeout per operation. The goal of IO#timeout is essentially a safety net. If #connect performs several internal operations, it's reasonable that there are several "timeouts". Other parts of IO behave similarly. e.g. each call to wait_whatever will use a new timeout, therefore, on slow connections, it's possible for things like gets to invoke read several times, and thus wait several times, each with a new timeout.

If users desire application level timeouts, e.g. "This group of operations should take at most X seconds", they should use Timeout.timeout or some similar construct.

@rhenium
Copy link
Member

rhenium commented Jan 17, 2024

This is superseded by #714 which fixes SSLSocket to correctly raise IO::Timeout.

@rhenium rhenium closed this Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants