From 40441f8ada8d4daf7ec9839224090931035f2589 Mon Sep 17 00:00:00 2001 From: John Spray Date: Wed, 8 Nov 2023 13:00:11 +0000 Subject: [PATCH] pageserver: use `Gate` for stronger safety check in `SlotGuard` (#5793) ## Problem #5711 and #5367 raced -- the `SlotGuard` type needs `Gate` to properly enforce its invariant that we may not drop an `Arc` from a slot. ## Summary of changes Replace the TODO with the intended check of Gate. --- libs/utils/src/sync/gate.rs | 7 +++++++ pageserver/src/tenant/mgr.rs | 9 +-------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/libs/utils/src/sync/gate.rs b/libs/utils/src/sync/gate.rs index 1391d238e604..9aad0af22d6c 100644 --- a/libs/utils/src/sync/gate.rs +++ b/libs/utils/src/sync/gate.rs @@ -85,6 +85,13 @@ impl Gate { warn_if_stuck(self.do_close(), &self.name, Duration::from_millis(1000)).await } + /// Check if [`Self::close()`] has finished waiting for all [`Self::enter()`] users to finish. This + /// is usually analoguous for "Did shutdown finish?" for types that include a Gate, whereas checking + /// the CancellationToken on such types is analogous to "Did shutdown start?" + pub fn close_complete(&self) -> bool { + self.sem.is_closed() + } + async fn do_close(&self) { tracing::debug!(gate = self.name, "Closing Gate..."); match self.sem.acquire_many(Self::MAX_UNITS).await { diff --git a/pageserver/src/tenant/mgr.rs b/pageserver/src/tenant/mgr.rs index 27f9d50c5401..07d161827221 100644 --- a/pageserver/src/tenant/mgr.rs +++ b/pageserver/src/tenant/mgr.rs @@ -1552,14 +1552,7 @@ impl SlotGuard { /// is responsible for protecting fn old_value_is_shutdown(&self) -> bool { match self.old_value.as_ref() { - Some(TenantSlot::Attached(tenant)) => { - // TODO: PR #5711 will add a gate that enables properly checking that - // shutdown completed. - matches!( - tenant.current_state(), - TenantState::Stopping { .. } | TenantState::Broken { .. } - ) - } + Some(TenantSlot::Attached(tenant)) => tenant.gate.close_complete(), Some(TenantSlot::Secondary) => { // TODO: when adding secondary mode tenants, this will check for shutdown // in the same way that we do for `Tenant` above