Skip to content

RUST-1437 Send endSessions on client shutdown #1216

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Oct 8, 2024
Merged

RUST-1437 Send endSessions on client shutdown #1216

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1fc8563
RUST-1437 Send endSessions on client shutdown
isabelatkinson Oct 3, 2024
97b52f3
sync logging test
isabelatkinson Oct 3, 2024
e81edbb
shut down test client on close
isabelatkinson Oct 4, 2024
130a7f6
fmt
isabelatkinson Oct 4, 2024
4f15d35
stronger debug assert
isabelatkinson Oct 7, 2024
fb28f77
i64 -> f64
isabelatkinson Oct 7, 2024
82c0d7a
use command monitoring
isabelatkinson Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fmt
  • Loading branch information
@isabelatkinson
isabelatkinson committed Oct 4, 2024
commit 130a7f679a76cd3b2c37672421e992b979016935
8 changes: 7 additions & 1 deletion src/test/spec/unified_runner/operation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.take().unwrap().shutdown().immediate(true).await;
client
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.client
.take()
.unwrap()
.shutdown()
.immediate(true)
.await;

let mut entities_to_remove = vec![];
for (key, value) in entities.iter() {
Expand Down