Improve MultiplexedConnection Error Handling #699
Conversation
jaymell
force-pushed
the
multiplex-connection-error-handling
branch
2 times, most recently
from
749720a
to
b7348d9
Compare
jaymell
force-pushed
the
multiplex-connection-error-handling
branch
2 times, most recently
from
a3bd58f
to
30e8578
Compare
|
If this looks okay, I believe we should consider merging. I think the current |
jaymell
force-pushed
the
multiplex-connection-error-handling
branch
2 times, most recently
from
a50c3ca
to
9ec146f
Compare
It was recently discovered that `aio::MultiplexedConnection` does not properly handle errors. If a command in the pipe fails, responses from subsequent commands in the pipe are not processed and will be returned to the next request issued on the connection, with potentially catastrophic results. This fix ensures that all pending commands in the pipe are processed before returning. Fixes redis-rs#698.
jaymell
force-pushed
the
multiplex-connection-error-handling
branch
from
9ec146f
to
4ff5e81
Compare
| // Need to gather more response values | ||
| return; | ||
| } | ||
| Err(err) => { |
There was a problem hiding this comment.
So do we typically expect other return items to come in even after an error?
There was a problem hiding this comment.
Yes. I will admit that intuitively it's a bit scary, but this is how the other Connections (e.g., for aio::Connection, and for the sync Connection) and indeed the other redis libraries I've looked at handle the situation. Unless there's an issue parsing the commands properly, or a larger infrastructure issue, the commands will all be sent and responses will be returned.
Again, I think the API itself here is poor. We really should return Results for individual commands rather than all-or-nothing but this fix needs to happen irrespective of that.
There was a problem hiding this comment.
We really should return Results for individual commands rather than all-or-nothing but this fix needs to happen irrespective of that.
I actually was looking for a FromRedisValue implementation for Result for this reason 😄
You might consider adding that as an option rather than pushing Result onto every consumer of your pipe api? Fwiw it's nice to be able to assert that the whole thing worked else it is an error. But for some use cases a partial success is important and may be distinct from an overall pipe failure - hence, a FromRedisValue<Result...> impl which would even let someone decide if 1 part of the pipeline is resulty distinct from the overall pipe!
Improve MultiplexedConnection Error Handling
It was recently discovered that
aio::MultiplexedConnectiondoes not properly handle errors. If a command in the pipe fails,
responses from subsequent commands in the pipe are not processed
and will be returned to the next request issued on the connection,
with potentially catastrophic results. This fix ensures that
all pending commands in the pipe are processed before returning.
Fixes #698.