RUST-1443 Ensure monitors close after server is removed from topology #744
Conversation
enforce minHeartbeatFrequencyMS when constructing via client (RUST-1476)
| @@ -1325,6 +1325,19 @@ impl ClientOptions { | |||
| } | |||
| } | |||
|
|
|||
| if let Some(heartbeat_frequency) = self.heartbeat_freq { | |||
There was a problem hiding this comment.
This validation existed, but it was in the URI parsing stage. I've moved it to ClientOptions::validate to ensure it gets hit even when the options are constructed directly.
| #[cfg(not(test))] | ||
| let min_frequency = MIN_HEARTBEAT_FREQUENCY; | ||
|
|
||
| tokio::select! { |
There was a problem hiding this comment.
I moved the minimum delay into wait_for_check_request to consolidate the checks for the server closing.
| .wait_for_check_request() | ||
| .await; | ||
| }; | ||
| tokio::pin!(wait_for_check_request); |
There was a problem hiding this comment.
we need the pin here since we're polling the same future repeatedly in a loop.
| /// topology. | ||
| #[cfg_attr(feature = "tokio-runtime", tokio::test(flavor = "multi_thread"))] | ||
| #[cfg_attr(feature = "async-std-runtime", async_std::test)] | ||
| async fn removed_server_monitor_stops() -> crate::error::Result<()> { |
There was a problem hiding this comment.
this test was copied pretty much as-is from #733
| @@ -1081,10 +1081,11 @@ pub(crate) struct TopologyCheckRequestReceiver { | |||
|
|
|||
| impl TopologyCheckRequestReceiver { | |||
There was a problem hiding this comment.
This was a minor change I made to mitigate the case where the value hasn't changed but there are still operations requesting an update. I don't believe this case actually could actually happen with the existing implementation, but I think it's best to make that clear here.
patrickfreed
requested review from
a team and
abr-egn
and removed request for
a team
src/sdam/monitor.rs
Outdated
| /// method will return. | ||
| /// | ||
| /// The `delay` parameter indicates how long this method should wait before listing to requests. | ||
| /// It is covered by the provided timeout. |
There was a problem hiding this comment.
does "covered by" mean "counts toward"? like if delay is 1s and timeout is 5s, the method delays for 1s then times out 4s if no requests come in? (I think yes from reading the logic)
There was a problem hiding this comment.
Yep, updated the docstring to more be clearer about this.
RUST-1443
This PR forward-ports the test added in #733 that verifies that monitors stop executing after their servers are removed from the topology. Running the test yielded a panic in the monitoring task due to a subtraction overflow (heartbeatFrequencyMS < minHeartbeatFrequencyMS), which made me notice we weren't validating heartbeatFrequencyMS when constructing a client via
ClientOptionsdirectly. I also fixed that here (RUST-1476).