Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PanicException does not subclass Exception #3918

Closed
stinodego opened this issue Mar 1, 2024 · 5 comments
Closed

PanicException does not subclass Exception #3918

stinodego opened this issue Mar 1, 2024 · 5 comments
Labels

Comments

@stinodego
Copy link

stinodego commented Mar 1, 2024

Bug Description

Python users cannot catch PanicException by catching the generic Exception. The following pattern is common:

try:
    ...
except Exception:
	# Do something in case any failures happen

It does subclass BaseException. However, the Python docs mention that exceptions should generally prefer to subclass Exception rather than BaseException.

programmers are encouraged to derive new exceptions from the Exception class or one of its subclasses, and not from BaseException.

Steps to Reproduce

  1. Compile a PyO3 library with the following flags:
pyo3 = { version = "0.20", features = ["abi3-py38", "extension-module", "multiple-pymethods"] }
  1. Cause a PanicException to be raised
  2. Try to catch it with an Exception

Backtrace

No response

Your operating system and version

Ubuntu 22.02

Your Python version (python --version)

Python 3.12

Your Rust version (rustc --version)

rustc 1.78.0-nightly (397937d81 2024-02-22)

Your PyO3 version

0.20

How did you install python? Did you use a virtualenv?

pyenv - virtual env

Additional Info

No response

@davidhewitt
Copy link
Member

There's a long history of this in #2783, #3057, and #3519.

The point is that Rust typically treats a panic as a fatal crash, and so we made a deliberate choice to inherit BaseException to model that same property.

We can definitely add additional options to PyO3 (mostly see #3519 for latest ideas) for libraries that want to think about panics differently. Though we would prefer that libraries also treated panics as fatal crashes (and did their best to minimise them).

@stinodego
Copy link
Author

Thank you for the explanation. I searched the tracker but somehow missed those issues. I'll close this then!

@stinodego stinodego closed this as not planned Won't fix, can't repro, duplicate, stale Mar 1, 2024
@mesner
Copy link

mesner commented Mar 1, 2024

The python multiprocessing and threading modules seem to assume basic handling of Exceptions (and not BaseException).

So if I run a task in a multiprocessing.pool.map() (or variant) that calls a function that raises a pyo3 panic, the thread / process hangs.

Even if this isn't supposed to occur, it seems to be a trap that limits compatibility with anything that assumes all errors will subclass Exception.

If this is true, then it seems that either the Panic should subclass Exception, or that all pyo3 libraries are expected to catch panics before they bubble up to consumers.

import polars as pl
import numpy as np
import multiprocessing as mp

def raiseError(i):
    raise pl.PolarsPanicError()

if __name__ == '__main__':
    with mp.Pool(2) as pool:
        pool.map(raiseError, [1,2])

    print("program complete")

see e.g.
https://github.com/python/cpython/blob/main/Lib/threading.py#L1006

@stinodego
Copy link
Author

all pyo3 libraries are expected to catch panics before they bubble up to consumers

For reference, Polars should do this. If a Polars user sees a panic, that's a bug and can be reported to our issue tracker.

@davidhewitt
Copy link
Member

The python multiprocessing and threading modules seem to assume basic handling of Exceptions (and not BaseException).

Thanks @mesner - that's definitely an interesting point to consider, as Rust does handle panics at thread boundaries. I'll think further on this...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants