Adds a connection health flag to the multiplexer #308
Conversation
|
The rationale behind this PR is establishing preconditions to multiplexer methods. At the moment,
This is very unlikely, but I'd like to avoid having to test it. With this PR, we guarantee that I've taken the opportunity and added a flag to signal whether our connection is healthy or not, which makes Let me know your views on this. I can fragment this in 2 PRs if you prefer. |
| mpx_.cancel_on_conn_lost(); | ||
| } | ||
|
|
||
| bool is_open() const noexcept { return stream_.is_open(); } |
There was a problem hiding this comment.
Can we deprecate is_open() as well? Or at least is should be renamed to is_healthy().
| conn_->mpx_.reset(); | ||
| clear_response(conn_->setup_resp_); | ||
| conn_->read_buffer_.clear(); | ||
| conn_->mpx_.on_connection_up(); |
There was a problem hiding this comment.
I would like to move away the implementation towards #104. For this PR that would mean mpx.on_connection_up() should be actually be called mpx.log_data_received() and be called somehow in redis_stream::async_read_some when the read size is different from 0. All my other comments in this PR assume this model.
Edit: perhaps it makes more sense to move health funktionality to the redis_sream where data is actually received.
There was a problem hiding this comment.
This would be equivalent to calling redis_stream::async_read_some with asio::cancel_after(ping_interval), and tearing down the connection if the timeout elapses, wouldn't it?
| // Handle connection health | ||
| void on_connection_up(); // Might be called in any state | ||
|
|
||
| void on_connection_down(); // Must be called once, with is_connection_healthy() == true |
There was a problem hiding this comment.
This function is not needed since the connection is considered down when data is older than the ping interval specified in config::health_check_interval.
| bool cancel_run_called_ = false; | ||
| usage usage_; | ||
| any_adapter receive_adapter_; | ||
| bool conn_healthy_ = false; |
There was a problem hiding this comment.
I think this could be a std::chrono::time_point that gets update at each log_data_received(). If creating a timestamp is two expensive i.e. I would need to think more.
|
|
||
| void on_connection_down(); // Must be called once, with is_connection_healthy() == true | ||
|
|
||
| bool is_connection_healthy() const { return conn_healthy_; } |
There was a problem hiding this comment.
This function should check if conn_healthy_ is not older than config::health_check_interval.
|
I've clearly tried to get too many things together in a single PR :) I'm going to split this in two: getting rid of the race condition with cancel_on_conn_lost and then improve the health checker. I need to think of your comments about the latter. |
|
A subset of this PR (including tests) is available in #309. It doesn't include any of the health checking functionality - it only solves the race condition problem. |
2 participants
Uses this flag in async_exec to make cancel_if_not_connected reliable
Deprecates basic_connection::run_is_canceled()
Introduces preconditions in multiplexer methods to be called by the reader and writer tasks
Ensures that request cancellation due to a connection lost is only called after the reader and writer have finished, avoiding potential race conditions
close #236