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

Dalli requires support for the connect_timeout option to TCPSocket#initialize #3421

Closed
nirvdrum opened this issue Jan 30, 2024 · 2 comments · Fixed by #3424
Closed

Dalli requires support for the connect_timeout option to TCPSocket#initialize #3421

nirvdrum opened this issue Jan 30, 2024 · 2 comments · Fixed by #3424
Assignees

Comments

@nirvdrum
Copy link
Collaborator

We currently do not support the connect_timeout option to TCPSocket#initialize. This has recently come up as a problem with Dalli 3.2.7, which started using connect_timeout. This will impact those running Rails with memcache as its cache store.

TypeError: no implicit conversion of {:connect_timeout=>1} into Integer
--
  | /usr/local/ruby/lib/truffle/socket/truffle.rb:168:in `coerce_to_string'
  | /usr/local/ruby/lib/truffle/socket/tcp_socket.rb:66:in `initialize'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/dalli-3.2.7/lib/dalli/socket.rb:93:in `open'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/dalli-3.2.7/lib/dalli/protocol/connection_manager.rb:210:in `memcached_socket'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/dalli-3.2.7/lib/dalli/protocol/connection_manager.rb:55:in `establish_connection'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/dalli-3.2.7/lib/dalli/protocol/base.rb:207:in `connect'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/dalli-3.2.7/lib/dalli/protocol/base.rb:196:in `ensure_connected!'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/dalli-3.2.7/lib/dalli/protocol/base.rb:58:in `alive?'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/dalli-3.2.7/lib/dalli/ring.rb:46:in `server_for_key'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/dalli-3.2.7/lib/dalli/client.rb:425:in `perform'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/dalli-3.2.7/lib/dalli/client.rb:208:in `set_cas'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/dalli-3.2.7/lib/dalli/client.rb:201:in `set'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/activesupport-7.1.3/lib/active_support/cache/mem_cache_store.rb:265:in `block (2 levels) in write_serialized_entry'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/connection_pool-2.4.1/lib/connection_pool.rb:110:in `block (2 levels) in with'
  | <internal:core> core/thread.rb:94:in `handle_interrupt'
  | <internal:core> core/thread.rb:94:in `handle_interrupt'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/connection_pool-2.4.1/lib/connection_pool.rb:109:in `block in with'
  | <internal:core> core/thread.rb:94:in `handle_interrupt'
  | <internal:core> core/thread.rb:94:in `handle_interrupt'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/connection_pool-2.4.1/lib/connection_pool.rb:106:in `with'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/activesupport-7.1.3/lib/active_support/cache/mem_cache_store.rb:265:in `block in write_serialized_entry'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/activesupport-7.1.3/lib/active_support/cache/mem_cache_store.rb:336:in `rescue_error_with'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/activesupport-7.1.3/lib/active_support/cache/mem_cache_store.rb:262:in `write_serialized_entry'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/activesupport-7.1.3/lib/active_support/cache/strategy/local_cache.rb:154:in `write_serialized_entry'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/activesupport-7.1.3/lib/active_support/cache/mem_cache_store.rb:252:in `write_entry'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/activesupport-7.1.3/lib/active_support/cache.rb:668:in `block in write'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/activesupport-7.1.3/lib/active_support/cache.rb:1030:in `block in _instrument'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/activesupport-7.1.3/lib/active_support/notifications.rb:208:in `instrument'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/activesupport-7.1.3/lib/active_support/cache.rb:1029:in `_instrument'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/activesupport-7.1.3/lib/active_support/cache.rb:1006:in `instrument'
  | /tmp/bundle/truffleruby/3.2.2.23.1.0/gems/activesupport-7.1.3/lib/active_support/cache.rb:666:in `write'
@rwstauner rwstauner self-assigned this Jan 30, 2024
@eregon
Copy link
Member

eregon commented Jan 31, 2024

Thanks for the report.
The corresponding CRuby ticket is https://bugs.ruby-lang.org/issues/17187.
From a quick look at the PR they seem to use the fact that socket file descriptors are immediately marked as non-blocking in CRuby and then try one connect() and if not immediately successful wait using select/poll with the timeout.

In lib/truffle/socket/tcp_socket.rb the logic seems to currently use a blocking socket and blocking connect() call.
So we can either do the same as CRuby but that might involve changing quite a bit of Socket code, or use Timeout.timeout instead, but that occurs the cost of an extra thread the first time it is used and likely has more overhead.

Likely relevant snippet from man 2 connect:

       EINPROGRESS
              The socket is nonblocking and the connection cannot be completed immediately.  (UNIX domain  sockets  failed
              with  EAGAIN  instead.)   It  is possible to select(2) or poll(2) for completion by selecting the socket for
              writing.  After select(2) indicates writability, use getsockopt(2) to read  the  SO_ERROR  option  at  level
              SOL_SOCKET  to  determine  whether  connect()  completed  successfully  (SO_ERROR is zero) or unsuccessfully
              (SO_ERROR is one of the usual error codes listed here, explaining the reason for the failure).

@eregon
Copy link
Member

eregon commented Jan 31, 2024

If this is aimed for 24.0 I think best to use the Timeout.timeout approach as it seems less risky (changing all socket fds to be non-blocking is likely a big change with many side effects).

rwstauner pushed a commit to Shopify/truffleruby that referenced this issue Jan 31, 2024
Co-authored-by: Manef Zahra <manef.zahra@shopify.com>
Co-authored-by: Patrick Lin <patrick.lin@shopify.com>
Co-authored-by: Kevin Menard <kevin.menard@shopify.com>
Co-authored-by: Randy Stauner <randy.stauner@shopify.com>

closes oracle#3421
rwstauner pushed a commit to rwstauner/truffleruby that referenced this issue Feb 1, 2024
Co-authored-by: Manef Zahra <manef.zahra@shopify.com>
Co-authored-by: Patrick Lin <patrick.lin@shopify.com>
Co-authored-by: Kevin Menard <kevin.menard@shopify.com>
Co-authored-by: Randy Stauner <randy.stauner@shopify.com>

closes oracle#3421
rwstauner pushed a commit to Shopify/truffleruby that referenced this issue Feb 2, 2024
Co-authored-by: Manef Zahra <manef.zahra@shopify.com>
Co-authored-by: Patrick Lin <patrick.lin@shopify.com>
Co-authored-by: Kevin Menard <kevin.menard@shopify.com>
Co-authored-by: Randy Stauner <randy.stauner@shopify.com>

closes oracle#3421
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants