RUST-1437 Send endSessions on client shutdown
#1216
Conversation
isabelatkinson
force-pushed
the
end-sessions
branch
from
d56f38d to
aa880f6
Compare
isabelatkinson
force-pushed
the
end-sessions
branch
from
aa880f6 to
e81edbb
Compare
| fn drop(&mut self) { | ||
| if !self.inner.shutdown.executed.load(Ordering::SeqCst) | ||
| && !self.inner.dropped.load(Ordering::SeqCst) | ||
| && TrackingArc::strong_count(&self.inner) == 1 |
There was a problem hiding this comment.
This implementation would make more sense on ClientInner, which will only drop when all copies of the client are released; however, the operation execution machinery is implemented on Client, which is needed to call run_command. Additionally, something like this doesn't work because drop only takes a mutable reference:
impl Drop for ClientInner {
fn drop(&mut self) {
if !self.shutdown.executed.load(Ordering::SeqCst) {
let client = Client {
// this can't take ownership
inner: TrackingArc::new(self),
}
...end sessions
}
}
}
| @@ -2182,7 +2182,13 @@ impl TestOperation for Close { | |||
| Entity::Client(_) => { | |||
| let client = entities.get_mut(id).unwrap().as_mut_client(); | |||
| let closed_client_topology_id = client.topology_id; | |||
| client.client = None; | |||
| client | |||
There was a problem hiding this comment.
Subtle bug - spawning a future in the client's drop that itself holds onto a client means that the actual ClientInner won't drop until the future finishes running. This is not a big problem, as the types contained in ClientInner don't require any server-side cleanup; however, the Topology emits some events upon close, and if the runtime shuts down before the spawned endSessions future finishes, those events won't be emitted (see here). The fix here is just to run shutdown in this close operation for Client so that no future is spawned when the client is dropped.
This did get me thinking that we could speed up these drop implementations by sending them as unacknowledged writes with moreToCome set, since we're ignoring the results anyway. We would need to add some functionality to the operation/message layer, but I do think this would decrease the likelihood of these spawned tasks not completing before the runtime shuts down.
No description provided.