paleolimbot commented on code in PR #1067: URL: https://github.com/apache/sedona-db/pull/1067#discussion_r3632887929
########## c/sedona-extension/src/runtime.rs: ########## @@ -0,0 +1,102 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! A Tokio runtime owner that shuts down in the background on drop. + +use std::mem::ManuallyDrop; +use std::ops::Deref; + +use tokio::runtime::Runtime; + +/// Owns a Tokio [`Runtime`] and shuts it down in the background when dropped. +/// +/// The default `Runtime::drop` blocks the dropping thread while it joins every +/// worker thread. When a runtime is shared through `Arc<RuntimeHandle>`, the +/// final reference can drop on any thread — including one attached to the +/// CPython interpreter (an ordinary decref or a cyclic-GC finalization). A +/// blocking native join keeps that thread from reaching a bytecode safe point, +/// which stalls interpreter-wide stop-the-world operations under a +/// free-threaded build. Dropping through this handle instead calls +/// [`Runtime::shutdown_background`], which returns immediately and lets the +/// worker threads wind down detached. All work submitted through +/// [`Runtime::block_on`] has already completed by the time the last handle +/// drops, so nothing is left in flight for the background shutdown to abandon. +/// +/// [`RuntimeHandle`] dereferences to the wrapped [`Runtime`], so callers use it +/// exactly as they would the runtime itself. +pub struct RuntimeHandle { + runtime: ManuallyDrop<Runtime>, +} + +impl RuntimeHandle { + /// Wrap a [`Runtime`] so that dropping it shuts down in the background. + pub fn new(runtime: Runtime) -> Self { + Self { + runtime: ManuallyDrop::new(runtime), + } + } +} + +impl Deref for RuntimeHandle { + type Target = Runtime; + + fn deref(&self) -> &Runtime { + &self.runtime + } +} + +impl Drop for RuntimeHandle { + fn drop(&mut self) { + // SAFETY: `runtime` is initialized in `new` and taken exactly once, + // here in `drop`; it is never accessed again afterward. + let runtime = unsafe { ManuallyDrop::take(&mut self.runtime) }; + runtime.shutdown_background(); Review Comment: I believe `shutdown_background()` can leak resources...while I'm sure that ongoing tasks *should* have finished by the time this happens, I'm not sure that is always true. How about a OnceLock "trash can" ...in this drop implementation? We launch a (non tokio) thread whose job is to drop the runtime (gracefully), and the trash can keeps track of the task. When a new runtimehandle is created, we check the trash and empty it of completed tasks. This approach gives us some visibility if we see consistently non-empty trash cans. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
