Skip to content
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.

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

RUST-1608 Clean shutdown for Client #920

Merged
merged 26 commits into from
Aug 10, 2023

Conversation

abr-egn
Copy link
Contributor

@abr-egn abr-egn commented Jul 24, 2023

RUST-1608

This adds the shutdown and shutdown_immediate methods on Client. Terminating the workers was straightforward but closing outstanding resources (cursors and sessions) was very much not. Initially, I was hoping that shutdown could actively track and clean up those when called, but the shared ownership required for that would have required API changes that were both backwards-incompatible and generally much less easy to work with.

What I've ended up with is using the pre-created tasks we discussed in RUST-1439; while this doesn't mean they're guaranteed to run when the runtime shuts down, it does mean that the Client can track and await them. Concretely, this allows shutdown to wait for all pending resources to have been dropped and complete async cleanup. This also fixes RUST-1099.

This solution does have a couple of downsides:

  • It pushes part of the "orderly shutdown" responsibility on to the user, requiring that all resources be dropped before shutdown will return. For normal usage, this shouldn't be an issue - Rust's typical scoped memory management should mean that most of the time that happens automatically, and when it doesn't it should be easy to fix. In case that's not possible, shutdown_immediate provides an escape hatch that does just the worker thread termination without the resource wait.
  • There's a potential race condition where the user could create a cursor or session after shutdown has waited for resources but before it closes out the workers. I think this mostly falls into the "please don't do that" sort of misbehavior.

src/client.rs Outdated Show resolved Hide resolved
src/client.rs Show resolved Hide resolved
src/client/session.rs Show resolved Hide resolved
src/client.rs Show resolved Hide resolved
src/id_set.rs Show resolved Hide resolved
src/tracking_arc.rs Show resolved Hide resolved
@abr-egn abr-egn marked this pull request as ready for review July 24, 2023 17:43
@abr-egn abr-egn requested a review from isabelatkinson July 24, 2023 17:43
src/client.rs Show resolved Hide resolved
src/client.rs Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/gridfs/upload.rs Show resolved Hide resolved
src/error.rs Show resolved Hide resolved
src/client.rs Show resolved Hide resolved
src/gridfs/upload.rs Show resolved Hide resolved
src/sync/client.rs Outdated Show resolved Hide resolved
src/test/client.rs Show resolved Hide resolved
Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

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

code changes look good, although it looks like there are some lint/test failures in CI

src/client.rs Show resolved Hide resolved
src/gridfs/upload.rs Show resolved Hide resolved
@abr-egn
Copy link
Contributor Author

abr-egn commented Jul 31, 2023

code changes look good, although it looks like there are some lint/test failures in CI

Ah, yep, fixed the lint, and I'd forgotten to restrict the transaction-using tests to transaction-supporting servers. The cargo-deny failure is unrelated (see RUST-1674 for details).

@abr-egn
Copy link
Contributor Author

abr-egn commented Jul 31, 2023

Updated with changes to unify the handling for shutdown and sync_worker. Beyond removing some duplication, this will also be very useful for implementing the "fill pool to min" method.

@abr-egn abr-egn requested a review from isabelatkinson August 10, 2023 12:55
Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

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

lgtm! nice work with this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants