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

feat: I/O Timeout #1025

Merged
merged 1 commit into from
Feb 7, 2025
Merged

feat: I/O Timeout #1025

merged 1 commit into from
Feb 7, 2025

Conversation

nickamorim
Copy link
Collaborator

Description

Currently we're relying on the SO_SNDTIMEO and SO_RCVTIMEO socket options for connect/read/write timeouts. As of CRuby 3.0, sockets are non-blocking by default which means that these socket options do not have any effect by default:

Timeouts only have effect for system calls that perform socket I/O (e.g., accept(2), connect(2), read(2), recvmsg(2), send(2), sendmsg(2)); timeouts have no effect for select(2), poll(2), epoll_wait(2), and so on.

https://man7.org/linux/man-pages/man7/socket.7.html

This is easily testable by using Toxiproxy to inject latency to an upstream Memcached:

  1. Follow the instructions to install Toxiproxy: https://github.com/Shopify/toxiproxy?tab=readme-ov-file#1-installing-toxiproxy
  2. memcached -d
  3. toxiproxy-cli create -l localhost:22122 -u localhost:11211 memcached_latency
  4. Inject 5s of latency: toxiproxy-cli toxic add -t latency -a latency=5000
  5. Create a Dalli client with a 50ms timeout: c = Dalli::Client.new("127.0.0.1:22122", socket_timeout: 0.05)
  6. Set a key: c.set("foo", "bar", 0, raw: true) and notice that it hangs for 10s (5s for the initial version call when establishing a connection and 5s for the set operation)

Then checkout nickamorim/io-timeout and you should get the following error:

W, [2024-11-11T13:18:10.996606 #84852]  WARN -- : 127.0.0.1:22122 failed (count: 0) IO::TimeoutError: Blocking operation timed out!
W, [2024-11-11T13:18:10.996684 #84852]  WARN -- : 127.0.0.1:22122 is down
dalli/lib/dalli/ring.rb:48:in `server_for_key': No server available (Dalli::RingError)
	dalli/lib/dalli/client.rb:441:in `perform'
	dalli/lib/dalli/client.rb:224:in `set_cas'
	dalli/lib/dalli/client.rb:217:in `set'

Proposed Solution

This PR uses the IO#timeout feature added in https://bugs.ruby-lang.org/issues/18630 to set a timeout for read, write, and connect calls.

Note: I understand that it's non-ideal to have a singular timeout for read, write, and connect calls. I can fix this in a follow-up PR.

Copy link
Owner

@petergoldstein petergoldstein left a comment

Choose a reason for hiding this comment

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

Please address the Rubocop failures. Otherwise this looks fine.

@nickamorim
Copy link
Collaborator Author

Please address the Rubocop failures.

I've been finding that the Rubocop rules are quite restrictive. Nonetheless they're now passing but we should revisit them.

@petergoldstein petergoldstein merged commit 159d3ec into main Feb 7, 2025
44 checks passed
@nickamorim nickamorim deleted the nickamorim/io-timeout branch February 7, 2025 14:14
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.

5 participants