search

contribsys / faktory_worker_ruby

Faktory worker for Ruby
GNU Lesser General Public License v3.0
214 stars 31 forks source link

Rescue OpenSSL errors in Client#transaction #83

Closed ibrahima closed 7 months ago

ibrahima commented 7 months ago

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.

ibrahima commented 7 months ago

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 commented 7 months ago

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

mperham commented 7 months ago

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

mperham commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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

ibrahima commented 7 months ago

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