RUST-1443 Stop executing server monitors after the server has been closed (2.3 backport) #733
Conversation
| /// 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 will need to be forward-ported to the main branch once RUST-360 lands.
patrickfreed
requested review from
a team and
abr-egn
and removed request for
a team
There was a problem hiding this comment.
SDAM stuff can be subtle, so to make sure I'm following what's going on:
Monitor's loop condition changed from topology liveness to server liveness (this is the core fix).- To enable
is_alivefor polling in the above,WorkerHandle[Listener]changed to usewatch::Receiver. TopologyWorkerpairsWorkerHandlewithServerin a map ofMonitoredServers; dropping those values is what will terminate the worker.- The server map is maintained in
sync_hosts;sync_hostsused to take a&mut TopologyStateto update, but that state is now rolled directly intoTopologyWorker.
Is that a reasonable summary?
src/sdam/description/topology/mod.rs
Outdated
| pub(crate) added_addresses: HashSet<&'a ServerAddress>, | ||
| pub(crate) changed_servers: | ||
| HashMap<&'a ServerAddress, (&'a ServerDescription, &'a ServerDescription)>, | ||
| pub(crate) removed_addresses: Vec<&'a ServerAddress>, |
There was a problem hiding this comment.
Is the change to Vec purely to allow sorting in tests? It looks like they're still "morally" sets, i.e. there won't be any duplicate keys. If so, it might be clearer to do the sorting when emitting the events in TopologyWorker::process_topology_diff and leave these as HashSet/Map.
There was a problem hiding this comment.
Good idea, done.
|
Yep, that's all exactly right. The change from using the |
src/sdam/test.rs
Outdated
| .sdam_event_handler(handler.clone() as Arc<dyn SdamEventHandler>) | ||
| .repl_set_name("foo".to_string()) | ||
| .build(); | ||
| options.heartbeat_freq = Some(Duration::from_millis(50)); |
There was a problem hiding this comment.
is this assignment necessary? looks like heartbeat_freq is already set by the builder
There was a problem hiding this comment.
good catch, fixed
patrickfreed
changed the title
RUST-1443
This PR fixes a bug where server monitors would continue executing even after a server had been removed from the topology.