apache / apache/arrow-rs

Preventing panics crossing exported C interfaces

Open
#8,913 2 comments 0 reactions 0 assignees View on GitHub
question
Dominant language
Rust
Stars
3.6k
Forks
1.3k
Avg merge
2d 16h
Merged PRs (30d)
168

Description

cc @kylebarron

**Which part is this question about**

Code base and/or documentation.

**Describe your question**

Should arrow-rs intercept panics before crossing the FFI boundary in the C bindings? If not, should it be a documented policy?

**Additional context**

Panics crossing foreign function interfaces is undefined behavior, usually causing the process to abort. An obvious example where this is undesirable is in computational kernel processes in Jupyter.

To prevent this, panics can be caught at the FFI boundary. For example, when binding Rust code to Python directly with PyO3, panics are caught and converted into [PanicExceptions](https://pyo3.rs/main/doc/pyo3/panic/struct.panicexception), allowing them to propagate all the way up the stack.

I recently encountered an initially surprising [issue](https://github.com/kylebarron/arro3/issues/460) exporting `RecordBatchReader`s to Python using `arro3`: panics were converted to PanicExceptions as expected when iterating using the exported Python object directly, but when the object was passed to PyArrow as a capsule, it would cause a process abort. Basically:

```python
reader = create_panicky_reader()
reader.read_next_batch() # raises PanicException

import pyarrow as pa
reader = pa.RecordBatchReader.from_stream(create_panicky_reader())
reader.read_next_batch() # Abort!
```

My understanding is that's because PyArrow is accessing the reader from native code using the C stream API, i.e. [here](https://github.com/apache/arrow-rs/blob/main/arrow-array/src/ffi_stream.rs#L236). In principle, panics could be intercepted at the interface and returned as [errors](https://arrow.apache.org/docs/format/CStreamInterface.html#error-codes) to the caller via this API.

My current workaround is to patch our `RecordBatchReader`s in Rust to catch panics and return them as `ArrowError`. I suppose this is fine, but then maybe it's worth documenting that it's the consumer's responsibility to place guardrails if abort-on-panic is not acceptable behavior.

Contributor guide

Open the contributing guide

Research direction

Start with arrow-array/src/ffi_stream.rs around line 236 and the linked C Stream Interface error-code documentation. Determine whether panics should be handled at the exported C boundary or documented as consumer responsibility; done means the project has a decided, documented policy and corresponding scope.

Written by the indexing model from the issue text.

Assessment

Tech stack
c, rust
Domain
api
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.