Panics are not handled gracefully after conversion to C stream pycapsule

[nvictus]

We encountered an issue where our exported RecordBatchReaders were causing process aborts when a panic was thrown in the Rust code during iteration. It turned out this was happening only when converting the arro3 record batch readers into their pyarrow equivalents using RecordBatchReader.from_stream. When pyarrow uses the C stream capsule from pyo3-arrow, unwinding panics are not getting caught before crossing the FFI barrier, causing Python to crash.

We have a workaround, but I thought I would flag this issue, as it surprised me at first and might be worth documenting.

I wonder if it makes sense to upstream a catch_unwind either in this project or in FFI_ArrowArrayStream, but I don't feel fully aware of the caveats of doing so. I assume this issue affects the other FFI bindings as well.

[kylebarron]

Do you have a reproducible example?

[kylebarron]

It's unclear from your example whether this is a bug in arro3.

I'm inclined to think it's a bug in your code: you need to be using result, not panic. It happened to work before for you because you were relying on pyo3's PanicException, but if your code gets called from within other Rust code, any panic will no longer be caught by that.

[nvictus]

you need to be using result, not panic

Absolutely! We avoid panics in our library but on rare occasions libraries we depend can panic during file parsing.

but if your code gets called from within other Rust code, any panic will no longer be caught by that

The code is getting called by PyArrow's extension layer, so it won't get caught by pyo3, because now the FFI boundary is between Rust and C++ (I believe).

So, I agree it may not be a bug in arro3 (or in arrow-rs), but maybe a caveat worth documenting.

I was able to reproduce with this minimal example:

pub struct PanickingReader {
    schema: arrow::datatypes::SchemaRef,
}

impl PanickingReader {
    fn new() -> Self {
        let schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, false)]));
        Self {
            schema,
        }
    }
}

impl Iterator for PanickingReader {
    type Item = Result<RecordBatch, ArrowError>;

    fn next(&mut self) -> Option<Self::Item> {
        panic!("Eek!");
    }
}

impl RecordBatchReader for PanickingReader {
    fn schema(&self) -> arrow::datatypes::SchemaRef {
        self.schema.clone()
    }
}

#[pyfunction]
pub fn create_panicking_reader() -> PyResult<PyRecordBatchReader> {
    Ok(PyRecordBatchReader::new(Box::new(PanickingReader::new())))
}

reader = create_panicking_reader()
pa.RecordBatchReader.from_stream(reader).read_next_batch()

[kylebarron]

Hmm. Perhaps it makes sense to create an issue in the upstream arrow-rs repo and then I can get other arrow maintainers thoughts