Skip to content

Commit 33c9a30

Browse files
authored
Audit panic handling in detached tasks (#8677)
## Rationale for this change Following #8672, I've audited a few other suspicous code paths that had similar shape, mostly on the read side. ## What changes are included in this PR? 1. Propagate panics and shutdown correctly for `FileSegmentSource`. 2. Handle panics correctly in the shared iterator used by DuckDB 3. Misc panic handling around our runtime abstraction. ## What APIs are changed? Are there any user-facing changes? None --------- Signed-off-by: Adam Gutglick <adam@spiraldb.com>
1 parent 4c762f7 commit 33c9a30

5 files changed

Lines changed: 650 additions & 42 deletions

File tree

vortex-duckdb/src/table_function.rs

Lines changed: 67 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,19 @@
44
use std::cmp::max;
55
use std::fmt::Formatter;
66
use std::fmt::{self};
7+
use std::pin::Pin;
78
use std::sync::Arc;
89
use std::sync::atomic::AtomicBool;
910
use std::sync::atomic::AtomicU64;
1011
use std::sync::atomic::Ordering;
12+
use std::task::Context;
13+
use std::task::Poll;
1114

1215
use custom_labels::CURRENT_LABELSET;
16+
use futures::FutureExt;
17+
use futures::Stream;
1318
use futures::StreamExt;
19+
use futures::future::BoxFuture;
1420
use itertools::Itertools;
1521
use num_traits::AsPrimitive;
1622
use static_assertions::assert_impl_all;
@@ -108,7 +114,8 @@ impl<'a> TableInitInput<'a> {
108114
}
109115
}
110116

111-
type DataSourceIterator = ThreadSafeIterator<VortexResult<(ArrayRef, Arc<ConversionCache>)>>;
117+
type ScanItem = VortexResult<(ArrayRef, Arc<ConversionCache>)>;
118+
type DataSourceIterator = ThreadSafeIterator<ScanItem>;
112119

113120
pub struct TableFunctionGlobal {
114121
iterator: DataSourceIterator,
@@ -268,10 +275,7 @@ pub fn init_global(init_input: &TableInitInput) -> VortexResult<TableFunctionGlo
268275
})
269276
.buffer_unordered(num_workers);
270277

271-
// Spawn a task to drive the partition stream and push array chunks into the channel.
272-
RUNTIME.handle().spawn(stream.collect::<()>()).detach();
273-
274-
let iterator = RUNTIME.block_on_stream_thread_safe(|_handle| rx.into_stream());
278+
let iterator = RUNTIME.block_on_stream_thread_safe(|_handle| scan_driver_stream(stream, rx));
275279

276280
Ok(TableFunctionGlobal {
277281
iterator,
@@ -283,6 +287,39 @@ pub fn init_global(init_input: &TableInitInput) -> VortexResult<TableFunctionGlo
283287
})
284288
}
285289

290+
fn scan_driver_stream<S>(stream: S, rx: kanal::AsyncReceiver<ScanItem>) -> ScanDriverStream
291+
where
292+
S: Stream<Item = ()> + Send + 'static,
293+
{
294+
ScanDriverStream {
295+
driver: Some(stream.collect::<()>().boxed()),
296+
rx: rx.into_stream().boxed(),
297+
}
298+
}
299+
300+
struct ScanDriverStream {
301+
driver: Option<BoxFuture<'static, ()>>,
302+
rx: futures::stream::BoxStream<'static, ScanItem>,
303+
}
304+
305+
impl Stream for ScanDriverStream {
306+
type Item = ScanItem;
307+
308+
fn poll_next(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> {
309+
let this = self.get_mut();
310+
if let Some(driver) = this.driver.as_mut()
311+
&& driver.as_mut().poll(cx).is_ready()
312+
{
313+
this.driver = None;
314+
}
315+
316+
match this.rx.as_mut().poll_next(cx) {
317+
Poll::Ready(None) if this.driver.is_some() => Poll::Pending,
318+
poll => poll,
319+
}
320+
}
321+
}
322+
286323
pub fn init_local(global: &TableFunctionGlobal) -> TableFunctionLocal {
287324
unsafe {
288325
use custom_labels::sys;
@@ -545,8 +582,11 @@ fn progress(bytes_read: &AtomicU64, bytes_total: &AtomicU64) -> f64 {
545582
mod tests {
546583
use std::sync::atomic::AtomicU64;
547584
use std::sync::atomic::Ordering::Relaxed;
585+
use std::task::Poll;
548586

587+
use crate::RUNTIME;
549588
use crate::table_function::progress;
589+
use crate::table_function::scan_driver_stream;
550590

551591
#[test]
552592
fn test_table_scan_progress() {
@@ -561,4 +601,26 @@ mod tests {
561601
bytes_total.fetch_add(100, Relaxed);
562602
assert!((progress(&bytes_read, &bytes_total) - 50.).abs() < f64::EPSILON);
563603
}
604+
605+
#[test]
606+
fn scan_driver_panic_propagates_through_iterator() {
607+
let (tx, rx) = kanal::bounded_async(1);
608+
let _tx = tx;
609+
let stream = futures::stream::poll_fn(|_| -> Poll<Option<()>> {
610+
panic!("duckdb scan driver panic");
611+
});
612+
613+
let mut iter =
614+
RUNTIME.block_on_stream_thread_safe(|_handle| scan_driver_stream(stream, rx));
615+
let panic = match std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| iter.next())) {
616+
Ok(_) => panic!("driver panic must propagate through iterator"),
617+
Err(panic) => panic,
618+
};
619+
let message = panic
620+
.downcast_ref::<&'static str>()
621+
.copied()
622+
.or_else(|| panic.downcast_ref::<String>().map(String::as_str))
623+
.unwrap_or("<unknown panic>");
624+
assert!(message.contains("duckdb scan driver panic"));
625+
}
564626
}

0 commit comments

Comments
 (0)