search

film42 / sidekiq-rs

A port of sidekiq to rust using tokio
MIT License
95 stars 10 forks source link

Refactor redis wrapper to use RedisError type instead of generic Box #25

Closed film42 closed 1 year ago

film42 commented 1 year ago

When an IO error is detected, we should be safe to assume the connection is down and needs to be recreated. Without this, it's possible for a process to be stuck in a loop that never recovers even though redis has come back online.

Updating the description:

Feel free to read the log below even though the real fix is found in #26 . Some of these changes are good and you can see how clippy fixes make the code easier and maybe even faster.

film42 commented 1 year ago

Seemed like a good time to migrate over to RedisError as well since it's more specific and works with everything in the custome redis wrapper.

RE: Checking for io errors from redis, this seems like the best way to detect a failure. The underlying TcpStream from tokio does not have a way to know if the stream was closed or not.

Example errors that were recovered:

Feb 12 06:32:17.328 ERRO Error leaked out the bottom: Broken pipe (os error 32)

Feb 12 06:34:36.897 ERRO Error leaked out the bottom: unexpected end of file

And the bb8 conn pool timeout is returned but is recoverable:

Feb 12 06:36:27.911 ERRO Error leaked out the bottom: TimedOut
film42 commented 1 year ago

Although I'm not sure why this was failing on windows subsystem for linux because the io error check is working, but for whatever reason, the check in redis-rs is not being used?

https://github.com/redis-rs/redis-rs/blob/0d0133ce43a65e4835645887f6cad1c73857b194/redis/src/aio.rs#L1110-L1119