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

rescue IO::TimeoutError raised by Ruby since 3.2.0 on blocking reads/writes #968

Closed
wants to merge 2 commits into from

Conversation

skaes
Copy link
Contributor

@skaes skaes commented Jul 20, 2023

One scenario in which this happens is when a firewall silently drops connections between clients and memcached instances.

With this patch the client reopens the connection and the request succeeds. Without it, IO:TimeoutError is raised for the application to handle which deviates from Dalli behavior under Ruby 3.1 and earlier.

There re several ways to patch this up. I opted for introducing an array of timeout classes for the rescue clauses.

@skaes
Copy link
Contributor Author

skaes commented Jul 20, 2023

See #967

@skaes
Copy link
Contributor Author

skaes commented Jul 20, 2023

Another option to fix this would be to use IOError in the rescue clauses, as IO::TimeoutError is a subclass of IOError and that class is defined for all Ruby versions.

@petergoldstein
Copy link
Owner

@skaes This looks ok to me, but it needs a rebase and a spec run. Can you please rebase the branch and we'll get CI running?

Thanks.

@skaes
Copy link
Contributor Author

skaes commented Sep 21, 2023

@petergoldstein the pull request was created from b69b24e which is the last commit on main. I don't know how to rebase it even more.

But really happy this gets addressed.

@petergoldstein
Copy link
Owner

@skaes You were correct - there was no rebase required when I made my last comment. But I just made some commits (primarily to address test failures). Would you be able to rebase now and we can get specs running?

Sorry about the confusion.

…writes

One scenario in which this happens is when a firewall silently drops
connections between clients and memcached instances.

With this patch the client reopens the connection and the request succeeds.
Without it, IO:TimeoutError is raised for the application to handle which
deviates from Dalli behavior under Ruby 3.1 and earlier.
@skaes skaes force-pushed the rescue-io-timeout-error branch from faec666 to 4acae51 Compare September 25, 2023 06:57
@skaes
Copy link
Contributor Author

skaes commented Sep 25, 2023

@petergoldstein rebased.

@petergoldstein
Copy link
Owner

@skaes Thanks for rebasing.

Can you please address the lints identified by Rubocop and get Rubocop passing? Then we can look at merging.

Thanks.

@skaes
Copy link
Contributor Author

skaes commented Sep 25, 2023

I fixed what I could. I left the task of commenting what module Protocol does to you, as I did not create that module.

@petergoldstein
Copy link
Owner

Merged in #973 with the final lint fixed. Thanks @skaes !

@skaes skaes deleted the rescue-io-timeout-error branch September 27, 2023 06:25
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.

2 participants