search

benashford / redis-async-rs

A Rust client for Redis, using Tokio
Apache License 2.0
251 stars 30 forks source link

Panic when an unexpected message is received #27

Open Emulator000 opened 6 years ago

Emulator000 commented 6 years ago

I noticed an issue and I can't explain why. My application was using this crate but I encountered a panic last night, inspecting the code I noticed this line: https://github.com/benashford/redis-async-rs/blob/a369e5b2fbe5f48c46c64065b89a56bcfc62eb8b/src/client/paired.rs#L134

Why there is a panic! here and not an error!? In the doc I can't see anything that says that the crate can panic in some circumstances.

Is this an expected behaviour? Can this be fixed?

benashford commented 6 years ago

This issue can only happen if, for some reason, Redis sends back more items of data than the client requests.

This can happen if certain commands, like MONITOR or SUBSCRIBE commands, are used with a PairedConnection (which is why there's a separate PubSubConnection) because those commands return unlimited amounts of data; and when this happens the connection cannot be used for general purposes, hence the need to flag the error.

I'm willing to change this, but I'm keen to avoid any hidden failures. Ideally the consumers will be informed, but this precise error only happens at the instant where nothings listening.

Potential solutions:

  1. Inform any later calls that the connection has errored and a new one will be required. Downside: forces the caller to do a lot of work.

  2. Drop and reconnect the connection automatically. (Which is what will happen with #28) Downside: runs the risk of the calling application never knowing that something went wrong in the first place, and therefore have a recurring difficult-to-understand bug.

  3. Have a list of banned commands, attempting to use them in PairedConnection will pre-emptively return an error, rather than waiting for the connection to become poisoned. Downside: will need to be continually updated with newer versions of Redis.

Emulator000 commented 6 years ago

@benashford The version of Redis that I'm using server-side is the 3.0 and I'm only using the expected right commands for the PairedConnection and not any other command that can run in multiple data, that's the weird thing here, maybe it's a bug.

What we can do as the best solution then? A part informing the caller that an unrecoverable error has occurred? We can't panic here without reporting it in the doc so, after a filter, we have to manage the case that the client receives an unexpected message anyway without panicking.

What do you think?

benashford commented 6 years ago

Yes, that does sound like it could be a bug. Has it only happened the once, or is it repeatable? And if it's repeatable is it possible to extract a test case?

Emulator000 commented 6 years ago

Unfortunately I can't try to reproduce it again because I'm running an important Telegram bot with an intensive use of SET command, running 24h a day...

I'm just using the SET command and anything else, I think that is a bug of the Redis version but this can't run in a panic client-side.

My fork it's currently running but I'm not monitoring the error, I can do it in the next release if this can help.

Emulator000 commented 3 years ago

@benashford I think that a panic! should be at least mentioned on the method doc but anyway, I think is not a correct behavior to panic the entire application in a library, an error is much better and let the implementer be able to handle it.