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 OpenSSL errors in Client#transaction #83

Merged
merged 1 commit into from
Feb 10, 2024

Conversation

ibrahima
Copy link
Contributor

@ibrahima ibrahima commented Feb 9, 2024

This feels like a similar category of socket/network-related errors that could be handled in a similar fashion.

This may help resolve the issues experienced in #51. Ultimately it probably indicates a network issue, but it seems like right now things get into a bad state if this exception is raised.

I don't know if e.g. jobs would get double-pushed if this handler were to handle this exception, but that is probably preferable to a process getting into a bad state until it's killed.

This feels like a similar category of socket/network-related errors that could be handled in a similar fashion.
@ibrahima
Copy link
Contributor Author

ibrahima commented Feb 9, 2024

Hmm... this might still be a little off the mark. It seems like something must be calling the OpenSSL shutdown function for OpenSSL::SSL::SSLError (SSL_read: shutdown while in init) to come up, and it seems like one of the times that FWR tries to close the socket is in this error handling case... so maybe transaction is the wrong place for this error handling?

@mperham
Copy link
Contributor

mperham commented Feb 9, 2024

Make sure you have a recent Redis version packaged with Faktory. I’m not sure which version is packaged in the Docker image.

@mperham
Copy link
Contributor

mperham commented Feb 9, 2024

I know there is a OpenSSLshutdown issue with earlier versions of Redis 6.2.x.

@mperham
Copy link
Contributor

mperham commented Feb 9, 2024

This issue is between the Faktory Ruby client and server so I guess a Redis upgrade isn’t exactly relevant but make sure you upgrade your Ruby version to get the latest OpenSSL fixes. I can see this happening if you have an old Ruby version using a newer OpenSSL v3.

@ibrahima
Copy link
Contributor Author

ibrahima commented Feb 9, 2024

Good tips, thanks! I will check those versions. We're on Ruby 3.2.2 so not the latest but not too old either.

@ibrahima
Copy link
Contributor Author

ibrahima commented Feb 9, 2024

Looks like the Docker image does have a pretty recent version of Redis:

/ # redis-server --version
Redis server v=7.0.14 sha=00000000:0 malloc=libc bits=64 build=35676b9e635fc646

And we're on Ruby 3.2.2 with OpenSSL 3.1.0 gem, and OpenSSL 3.0.2 apt package (Ubuntu 22.04). So pretty recent versions although not the latest.

@mperham mperham merged commit e163c28 into contribsys:main Feb 10, 2024
5 checks passed
@ibrahima ibrahima deleted the patch-1 branch February 12, 2024 19:11
@mperham
Copy link
Contributor

mperham commented Feb 14, 2024

I think this will break if the user is not using TLS because we conditionally require OpenSSL. That error class won't exist.

mperham added a commit that referenced this pull request Feb 14, 2024
@ibrahima
Copy link
Contributor Author

Ah good call, I didn't realize it was conditionally required. Thanks for fixing that!

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