Skip to content

Commit 40e6746

Browse files
fix: update TokioBackgroundExecutor to join thread instead of detaching (delta-io#2126)
## What changes are proposed in this pull request? The `TokioBackgroundExecutor` detaches its background thread on drop, which can cause a race condition with process exit. We hit this in a Postgres extension where the detached thread would conflict with Postgres backend exit. This udpates `JoiningBackgroundExecutor` to join its thread instead of deatching it, ensuring all runtime cleanup completes before the executor is destroyed. We have currenlty implemented this locally but would like to upstream it. ## How was this change tested? - Existing executor tests
1 parent 69bf275 commit 40e6746

1 file changed

Lines changed: 24 additions & 4 deletions

File tree

kernel/src/engine/default/executor.rs

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -54,18 +54,38 @@ pub mod tokio {
5454
use super::TaskExecutor;
5555
use futures::TryFutureExt;
5656
use futures::{future::BoxFuture, Future};
57+
use std::mem::ManuallyDrop;
5758
use std::sync::mpsc::channel;
5859
use tokio::runtime::{EnterGuard, Handle, RuntimeFlavor};
5960

6061
use crate::{DeltaResult, Error};
6162

6263
/// A [`TaskExecutor`] that uses the tokio single-threaded runtime in a
6364
/// background thread to service tasks.
65+
///
66+
/// On drop, the background thread is joined to ensure the runtime is fully
67+
/// shut down before the executor is destroyed.
6468
#[derive(Debug)]
6569
pub struct TokioBackgroundExecutor {
66-
sender: tokio::sync::mpsc::Sender<BoxFuture<'static, ()>>,
70+
sender: ManuallyDrop<tokio::sync::mpsc::Sender<BoxFuture<'static, ()>>>,
6771
handle: Handle,
68-
_thread: std::thread::JoinHandle<()>,
72+
/// `Option` because `join` takes ownership; we `take` it in `Drop` to move the
73+
/// handle out. Never `None` outside of `Drop`.
74+
thread: Option<std::thread::JoinHandle<()>>,
75+
}
76+
77+
impl Drop for TokioBackgroundExecutor {
78+
fn drop(&mut self) {
79+
// SAFETY: The inner `Sender` has not been dropped yet because this is
80+
// the only drop site and `Drop::drop` runs exactly once.
81+
// Drop sender first to close the channel, signaling the background
82+
// thread to exit its recv loop.
83+
unsafe { ManuallyDrop::drop(&mut self.sender) };
84+
// Join the thread so that runtime shutdown completes before we return.
85+
if let Some(thread) = self.thread.take() {
86+
let _ = thread.join();
87+
}
88+
}
6989
}
7090

7191
impl Default for TokioBackgroundExecutor {
@@ -93,9 +113,9 @@ pub mod tokio {
93113
});
94114
let handle = handle_receiver.recv().unwrap();
95115
Self {
96-
sender,
116+
sender: ManuallyDrop::new(sender),
97117
handle,
98-
_thread: thread,
118+
thread: Some(thread),
99119
}
100120
}
101121
}

0 commit comments

Comments
 (0)