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

SystemError: <method 'throw' of 'coroutine' objects> returned NULL without setting an exception #2860

Closed
ashb opened this issue Jan 5, 2023 · 12 comments

Comments

@ashb
Copy link

ashb commented Jan 5, 2023

Bug Description

This is a slightly odd one that I've managed to reduce down to a small(ish) reproduction case.

If you call back into python code from inside Drop whilst in the exit block of asyncio.TaskGenerator we get a SystemError.

While I am using the pyo3-asyncio crate (v0.17) I don't think this in that create.

If you remove the _ = py.run("a = 1", None, None); call inside the fn drop the problem goes away. I feel like this problem might be possible to reproduce with a normal context manager to. If I can get that to happen I will update this bug.

Steps to Reproduce

Cargo.toml

[package]
name = "test"
version = "0.1.0"
edition = "2021"

[lib]
name = "logtest"
crate-type = ["cdylib"]

[dependencies]
pyo3 = { version = "0.17.3", features = ["extension-module"] }
pyo3-asyncio = { version = "0.17", features = ["async-std-runtime"] }
async-std = "1.12"
backtrace = "*"

src/lib.rs:

use std::sync::{atomic::AtomicUsize, Arc};

use pyo3::{prelude::*, pymethods};

#[pyclass]
struct SomeAwaitable(Arc<AtomicUsize>);

#[pymethods]
impl SomeAwaitable {
    #[new]
    pub fn __new__() -> Self {
        SomeAwaitable(Arc::new(AtomicUsize::new(1)))
    }

    fn __aiter__(slf: PyRef<'_, Self>) -> PyRef<'_, Self> {
        slf
    }

    pub fn __anext__<'a>(&'a mut self, py: Python<'a>) -> PyResult<Option<&'a PyAny>> {
        match self._next_event(py) {
            Ok(event) => Ok(Some(event)),
            Err(err) => Err(err),
        }
    }

    fn _next_event<'a>(&'a mut self, py: Python<'a>) -> PyResult<&'a PyAny> {
        let val = Arc::clone(&self.0);
        pyo3_asyncio::async_std::future_into_py(py, async move {
            async_std::task::sleep(std::time::Duration::from_secs(1)).await;
            let for_py = val.fetch_add(1, std::sync::atomic::Ordering::SeqCst);
            Ok(Python::with_gil(|py| for_py.into_py(py)))
        })
    }
}

impl Drop for SomeAwaitable {
    fn drop(&mut self) {
        let bt = backtrace::Backtrace::new();

        println!("Rust Backtrace:\n{:?}", bt);
        Python::with_gil(|py| {
            /// HERE: comment this out and the error goes away
            _ = py.run("a = 1", None, None);
        });
    }
}

/// A Python module implemented in Rust.
#[pymodule]
fn logtest(_py: Python, m: &PyModule) -> PyResult<()> {
    m.add_class::<SomeAwaitable>()?;
    Ok(())
}

runner.py:

import logtest
import asyncio


async def watch_volume():
    async def simple_test():
        # await asyncio.sleep(10)
        async for event in logtest.SomeAwaitable():
            ...

    async with asyncio.TaskGroup() as tg:
        tg.create_task(simple_test())


try:
    asyncio.run(asyncio.wait_for(watch_volume(), timeout=2.5))
except asyncio.TimeoutError:
    pass

Backtrace

Rust Backtrace (generated manually using the backtrace create, it didn't actually panic with a BT):

   0: backtrace::backtrace::dbghelp::trace
             at C:\Users\Ash\.cargo\registry\src\github.com-1ecc6299db9ec823\backtrace-0.3.67\src\backtrace\dbghelp.rs:98
   1: backtrace::backtrace::trace_unsynchronized<backtrace::capture::impl$1::create::closure_env$0>
             at C:\Users\Ash\.cargo\registry\src\github.com-1ecc6299db9ec823\backtrace-0.3.67\src\backtrace\mod.rs:66
   2: backtrace::backtrace::trace<backtrace::capture::impl$1::create::closure_env$0>
             at C:\Users\Ash\.cargo\registry\src\github.com-1ecc6299db9ec823\backtrace-0.3.67\src\backtrace\mod.rs:53
   3: backtrace::capture::Backtrace::create
             at C:\Users\Ash\.cargo\registry\src\github.com-1ecc6299db9ec823\backtrace-0.3.67\src\capture.rs:176
   4: backtrace::capture::Backtrace::new
             at C:\Users\Ash\.cargo\registry\src\github.com-1ecc6299db9ec823\backtrace-0.3.67\src\capture.rs:140
   5: logtest::impl$0::drop
             at src\lib.rs:38
   6: core::ptr::drop_in_place<logtest::SomeAwaitable>
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52\library\core\src\ptr\mod.rs:487
   7: core::ptr::drop_in_place<core::cell::UnsafeCell<logtest::SomeAwaitable> >
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52\library\core\src\ptr\mod.rs:487
   8: core::mem::manually_drop::ManuallyDrop<core::cell::UnsafeCell<logtest::SomeAwaitable> >::drop<core::cell::UnsafeCell<logtest::SomeAwaitable> >
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52\library\core\src\mem\manually_drop.rs:144
   9: pyo3::pycell::impl$38::tp_dealloc<logtest::SomeAwaitable>
             at C:\Users\Ash\.cargo\registry\src\github.com-1ecc6299db9ec823\pyo3-0.17.3\src\pycell.rs:903
  10: pyo3::impl_::pyclass::tp_dealloc::closure$0<logtest::SomeAwaitable>
             at C:\Users\Ash\.cargo\registry\src\github.com-1ecc6299db9ec823\pyo3-0.17.3\src\impl_\pyclass.rs:1111
  11: pyo3::callback::handle_panic::closure$0<pyo3::impl_::pyclass::tp_dealloc::closure_env$0<logtest::SomeAwaitable>,tuple$<> >
             at C:\Users\Ash\.cargo\registry\src\github.com-1ecc6299db9ec823\pyo3-0.17.3\src\callback.rs:252
  12: std::panicking::try::do_call<pyo3::callback::handle_panic::closure_env$0<pyo3::impl_::pyclass::tp_dealloc::closure_env$0<logtest::SomeAwaitable>,tuple$<> >,enum$<core::result::Result<tuple$<>,pyo3::err::PyErr> > >
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52\library\std\src\panicking.rs:492
  13: std::panicking::try::do_catch<logtest::_::impl$1::__pymethod__next_event__::closure_env$0,enum$<core::result::Result<ptr_mut$<pyo3_ffi::object::PyObject>,pyo3::err::PyErr> > >
  14: std::panicking::try<enum$<core::result::Result<tuple$<>,pyo3::err::PyErr> >,pyo3::callback::handle_panic::closure_env$0<pyo3::impl_::pyclass::tp_dealloc::closure_env$0<logtest::SomeAwaitable>,tuple$<> > >
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52\library\std\src\panicking.rs:456
  15: std::panic::catch_unwind<pyo3::callback::handle_panic::closure_env$0<pyo3::impl_::pyclass::tp_dealloc::closure_env$0<logtest::SomeAwaitable>,tuple$<> >,enum$<core::result::Result<tuple$<>,pyo3::err::PyErr> > >
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52\library\std\src\panic.rs:137
  16: pyo3::callback::handle_panic<pyo3::impl_::pyclass::tp_dealloc::closure_env$0<logtest::SomeAwaitable>,tuple$<> >
             at C:\Users\Ash\.cargo\registry\src\github.com-1ecc6299db9ec823\pyo3-0.17.3\src\callback.rs:252
  17: pyo3::impl_::pyclass::tp_dealloc<logtest::SomeAwaitable>
             at C:\Users\Ash\.cargo\registry\src\github.com-1ecc6299db9ec823\pyo3-0.17.3\src\impl_\pyclass.rs:1111
  18: PyEval_EvalFrameDefault
  19: PyObject_GetBuffer
  20: PyErr_ExceptionMatches
  21: PyErr_TrySetFromCause
  22: PyErr_TrySetFromCause
  23: PyObject_GetMethod
  24: PyObject_VectorcallMethod
  25: PyInit__asyncio
  26: PyInit__asyncio
  27: PyInit__asyncio
  28: PyInit__asyncio
  29: PyIter_Send
  30: PyContext_NewHamtForTests
  31: PyContext_NewHamtForTests
  32: PyUnicode_RichCompare
  33: PyObject_Call
  34: PyList_AsTuple
  35: PyEval_EvalFrameDefault
  36: PyMapping_Check
  37: PyEval_EvalCode
  38: PyArena_Free
  39: PyArena_Free
  40: PyThread_tss_is_created
  41: PyRun_SimpleFileObject
  42: PyRun_AnyFileObject
  43: PySys_GetSizeOf
  44: PySys_GetSizeOf
  45: Py_RunMain
  46: Py_RunMain
  47: Py_Main
  48: <unknown>
  49: BaseThreadInitThunk
  50: RtlUserThreadStart

Python traceback

  + Exception Group Traceback (most recent call last):
  |   File "C:\Users\Ash\code\logtest\runner.py", line 16, in <module>
  |     asyncio.run(asyncio.wait_for(watch_volume(), timeout=2.5))
  |   File "C:\Users\Ash\AppData\Local\Programs\Python\Python311\Lib\asyncio\runners.py", line 190, in run
  |     return runner.run(main)
  |            ^^^^^^^^^^^^^^^^
  |   File "C:\Users\Ash\AppData\Local\Programs\Python\Python311\Lib\asyncio\runners.py", line 118, in run
  |     return self._loop.run_until_complete(task)
  |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |   File "C:\Users\Ash\AppData\Local\Programs\Python\Python311\Lib\asyncio\base_events.py", line 653, in run_until_complete
  |     return future.result()
  |            ^^^^^^^^^^^^^^^
  |   File "C:\Users\Ash\AppData\Local\Programs\Python\Python311\Lib\asyncio\tasks.py", line 490, in wait_for
  |     return fut.result()
  |            ^^^^^^^^^^^^
  |   File "C:\Users\Ash\code\logtest\runner.py", line 11, in watch_volume
  |     async with asyncio.TaskGroup() as tg:
  |   File "C:\Users\Ash\AppData\Local\Programs\Python\Python311\Lib\asyncio\taskgroups.py", line 133, in __aexit__
  |     raise me from None
  | ExceptionGroup: unhandled errors in a TaskGroup (1 sub-exception)
  +-+---------------- 1 ----------------
    | SystemError: <method 'throw' of 'coroutine' objects> returned NULL without setting an exception

Your operating system and version

Windows 11

Your Python version (python --version)

Python 3.11.1

Your Rust version (rustc --version)

1.64.0 (a55dd71d5 2022-09-19

Your PyO3 version

0.17.3

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

python.org installer + python -m venv to create venv

Additional Info

No response

@ashb ashb added the bug label Jan 5, 2023
@ashb
Copy link
Author

ashb commented Jan 5, 2023

The way I first ran in to this problem was actually by using pyo3-log and I had debug!() call in my Drop function.

Is this fixable, or is the best "fix" to document "don't call to the gil form inside Drop?"?

Possibly related to #2479

@ashb
Copy link
Author

ashb commented Jan 5, 2023

This is very particular to the scoping/object lifetimes,

For instance if I change the example (and my real-repro case I reduced this down from) from this:

    async def simple_test():
        async for event in logtest.SomeAwaitable():
            ...

to

    async def simple_test():
        x = logtest.SomeAwaitable()
        async for event in x:
           ... 

The SystemError goes away. This is way beyond my cpython internals and rust knowledge to be able to dig much further.

@davidhewitt
Copy link
Member

Interesting edge case. It looks like asyncio raises a CancelledError, and then your drop code runs while this error is unwinding Python stack frames.

The py.run call immediately fails and returns this exception in a PyErr, which you throw away with the _ = assignment. If you change the line to

if let Err(e) = py.run("a = 1", None, None) {
    e.restore(py);
}

then you're passing the error back to Python again and your crash goes away.

Two possible thoughts here:

  • We should probably improve documentation on what to do in cases like drop when it's not possible to return PyResult.
  • Is it a bug that py.run fails immediately with the existing exception? Probably, I think it's surprising. Need to think about whether we can / should change things here.

@ashb
Copy link
Author

ashb commented Jan 10, 2023

The py.run call immediately fails and returns this exception in a PyErr

Does this mean it is impossible to use pyo3-log in a Drop call in such a circumstance?

https://github.com/vorner/pyo3-log/blob/b793ce645d68ace9ba8df53d219db1377474d8da/src/lib.rs#L540-L543 was the first place in pyo3-log that hit this error.

@davidhewitt
Copy link
Member

Does this mean it is impossible to use pyo3-log in a Drop call in such a circumstance?

At present, yes. It would be worth reporting as an issue in pyo3-log, as the workaround I suggest above could be added there.

I'm also open to discussion if PyO3 should change this behaviour internally so that the ecosystem doesn't need to workaround this.

@carlos-rian
Copy link

I have a similar bug, but it's inconsistent; I can't reproduce it at any time.
I got this error: SystemError: <method 'send' of 'coroutine' objects> returned NULL without setting an exception

I tried using async code with uvicorn + FastAPI as loop.

traceback:

Traceback (most recent call last):
  File "/home/carlos-rian/.cache/pypoetry/virtualenvs/qd-web-app-loIbxImP-py3.10/lib/python3.10/site-packages/uvicorn/protocols/http/h11_impl.py", line 407, in run_asgi
    result = await app(  # type: ignore[func-returns-value]
  File "/home/carlos-rian/.cache/pypoetry/virtualenvs/qd-web-app-loIbxImP-py3.10/lib/python3.10/site-packages/uvicorn/middleware/proxy_headers.py", line 78, in __call__
    return await self.app(scope, receive, send)
  File "/home/carlos-rian/.cache/pypoetry/virtualenvs/qd-web-app-loIbxImP-py3.10/lib/python3.10/site-packages/fastapi/applications.py", line 270, in __call__
    await super().__call__(scope, receive, send)
  File "/home/carlos-rian/.cache/pypoetry/virtualenvs/qd-web-app-loIbxImP-py3.10/lib/python3.10/site-packages/starlette/applications.py", line 124, in __call__
    await self.middleware_stack(scope, receive, send)
  File "/home/carlos-rian/.cache/pypoetry/virtualenvs/qd-web-app-loIbxImP-py3.10/lib/python3.10/site-packages/starlette/middleware/errors.py", line 184, in __call__
    raise exc
  File "/home/carlos-rian/.cache/pypoetry/virtualenvs/qd-web-app-loIbxImP-py3.10/lib/python3.10/site-packages/starlette/middleware/errors.py", line 162, in __call__
    await self.app(scope, receive, _send)
  File "/home/carlos-rian/.cache/pypoetry/virtualenvs/qd-web-app-loIbxImP-py3.10/lib/python3.10/site-packages/starlette/middleware/cors.py", line 84, in __call__
    await self.app(scope, receive, send)
  File "/home/carlos-rian/.cache/pypoetry/virtualenvs/qd-web-app-loIbxImP-py3.10/lib/python3.10/site-packages/asgi_correlation_id/middleware.py", line 81, in __call__
    await self.app(scope, receive, handle_outgoing_request)
  File "/home/carlos-rian/.cache/pypoetry/virtualenvs/qd-web-app-loIbxImP-py3.10/lib/python3.10/site-packages/starlette/middleware/exceptions.py", line 75, in __call__
    raise exc
  File "/home/carlos-rian/.cache/pypoetry/virtualenvs/qd-web-app-loIbxImP-py3.10/lib/python3.10/site-packages/starlette/middleware/exceptions.py", line 64, in __call__
    await self.app(scope, receive, sender)
  File "/home/carlos-rian/.cache/pypoetry/virtualenvs/qd-web-app-loIbxImP-py3.10/lib/python3.10/site-packages/fastapi/middleware/asyncexitstack.py", line 21, in __call__
    raise e
  File "/home/carlos-rian/.cache/pypoetry/virtualenvs/qd-web-app-loIbxImP-py3.10/lib/python3.10/site-packages/fastapi/middleware/asyncexitstack.py", line 18, in __call__
    await self.app(scope, receive, send)
  File "/home/carlos-rian/.cache/pypoetry/virtualenvs/qd-web-app-loIbxImP-py3.10/lib/python3.10/site-packages/starlette/routing.py", line 680, in __call__
    await route.handle(scope, receive, send)
  File "/home/carlos-rian/.cache/pypoetry/virtualenvs/qd-web-app-loIbxImP-py3.10/lib/python3.10/site-packages/starlette/routing.py", line 275, in handle
    await self.app(scope, receive, send)
  File "/home/carlos-rian/.cache/pypoetry/virtualenvs/qd-web-app-loIbxImP-py3.10/lib/python3.10/site-packages/starlette/routing.py", line 65, in app
    response = await func(request)
  File "/home/carlos-rian/.cache/pypoetry/virtualenvs/qd-web-app-loIbxImP-py3.10/lib/python3.10/site-packages/fastapi/routing.py", line 235, in app
    raw_response = await run_endpoint_function(
  File "/home/carlos-rian/.cache/pypoetry/virtualenvs/qd-web-app-loIbxImP-py3.10/lib/python3.10/site-packages/fastapi/routing.py", line 161, in run_endpoint_function
    return await dependant.call(**values)
  File "/mnt/c/Users/carlos-rian/Documents/Repositories/qd-webapp-python-template/./app/resource/item.py", line 34, in put_item
    return await item.put_item(id=id, data=data, headers=headers)  <<<<<  this method call the compiled pyo3 rust function.
SystemError: <method 'send' of 'coroutine' objects> returned NULL without setting an exception

@davidhewitt
Copy link
Member

@carlos-rian despite the inconsistency do you think you can reduce it to a repro which triggers it given enough iterations? It's quite possible it's the same issue as the one described above, but it would be great to confirm.

@carlos-rian
Copy link

@davidhewitt I can try.

It has some dependency, but it's synchronous.

This is the async part.

conn.rs

use convert::convert_result_set;
use py_types::PyRow;
use py_types::PyRows;
use py_types::{py_error, DBError, PySQLXError, PySQLXResult};
use pyo3::prelude::*;
use quaint::connector::IsolationLevel;
use quaint::prelude::*;
use quaint::single::Quaint;

#[pyclass]
#[derive(Debug, Clone)]
pub struct Connection {
    conn: Quaint,
}

impl Connection {
    // create a new connection using the given url
    pub async fn new(uri: String) -> Result<Self, PySQLXError> {
        let conn = match Quaint::new(uri.as_str()).await {
            Ok(r) => r,
            Err(e) => return Err(py_error(e, DBError::ConnectError)),
        };
        Ok(Self { conn })
    }

    // Execute a query given as SQL, interpolating the given parameters. return a PySQLXResult
    async fn _query(&self, sql: &str) -> Result<PySQLXResult, PySQLXError> {
        match self.conn.query_raw(sql, &[]).await {
            Ok(r) => Ok(convert_result_set(r)),
            Err(e) => Err(py_error(e, DBError::QueryError)),
        }
    }

    // Execute a query given as SQL, interpolating the given parameters and returning the number of affected rows.
    async fn _execute(&self, sql: &str) -> Result<u64, PySQLXError> {
        match self.conn.execute_raw(sql, &[]).await {
            Ok(r) => Ok(r),
            Err(e) => Err(py_error(e, DBError::ExecuteError)),
        }
    }
}

#[pymethods]
impl Connection {
    pub fn query<'a>(&self, py: Python<'a>, sql: String) -> PyResult<&'a PyAny> {
        let slf = self.clone();
        pyo3_asyncio::tokio::future_into_py(py, async move {
            match slf._query(sql.as_str()).await {
                Ok(r) => Ok(r),
                Err(e) => Err(e.to_pyerr()),
            }
        })
    }

    pub fn execute<'a>(&mut self, py: Python<'a>, sql: String) -> PyResult<&'a PyAny> {
        let slf = self.clone();
        pyo3_asyncio::tokio::future_into_py(py, async move {
            match slf._execute(sql.as_str()).await {
                Ok(r) => Python::with_gil(|py| Ok(r.to_object(py))),
                Err(e) => Err(e.to_pyerr()),
            }
        })
    }
}

lib.rs

use database::Connection;
//use py_types::{PySQLXError, PySQLXResult};

use pyo3::prelude::*;
use pyo3::wrap_pyfunction;

pub fn get_version() -> String {
    let version = env!("CARGO_PKG_VERSION").to_string();
    version.replace("-alpha", "a").replace("-beta", "b")
}

#[pyfunction]
fn new(py: Python, uri: String) -> PyResult<&PyAny> {
    pyo3_asyncio::tokio::future_into_py(py, async move {
        match Connection::new(uri).await {
            Ok(r) => Ok(r),
            Err(e) => Err(e.to_pyerr()),
        }
    })
}

#[pymodule]
fn pysqlx_core(_py: Python, m: &PyModule) -> PyResult<()> {
    m.add("__version__", get_version())?;
    m.add_function(wrap_pyfunction!(new, m)?)?;
    m.add_class::<Connection>()?;
   //m.add_class::<PySQLXResult>()?;
    //m.add_class::<PySQLXError>()?;
    Ok(())
}

main.py

conn = await pysqlx_core.new(uri="sqlite:./db.db")

await conn.execute("insert into ...")

await conn.query("select * from ...")

The event loop used to test is uvloop.

Complete rust project: https://github.com/carlos-rian/pysqlx-core

Python wrapper project: https://github.com/carlos-rian/pysqlx-engine

FastAPI + Uvicorn

from fastapi import FastAPI
from pysqlx_engine import PySQLXEngine

class ItemDB:
    def __init__(self, uri: str) -> None:
        self.db = PySQLXEngine(uri=uri)

    def __await__(self):
        async def _closure():
            await self.db.connect()
            return self

        return _closure().__await__()

    async def update(self, id):
        sql = """
            UPDATE tmp.Items SET ... WHERE id = :id
        """
        params = { "id": id}
        await self.db.execute(sql=sql, parameters=params)
        return await self.db.query("select * from tmp.Items where id = :id", parameters=params)

app = FastAPI()

@app.put("/items/{id}")
async def update_item(id: int):
    db = await ItemDB(uri="...")
    resp = await db.update(id=id)
    return resp


if __name__ == "__main__":
    import uvicorn

    uvicorn.run(app)

Sorry about that. The project is a bit big.

So I had to add a lot of code.

Thanks.

@davidhewitt
Copy link
Member

So I'll be honest, I won't be able to find the time to figure out how to run that and trigger the bug you're experiencing. If you want me to debug this, please push it to a git repo, ideally removing everything that's not relevant, and include a script which I can launch which triggers the bug (even if it's just running a loop repeatedly until the bug strikes).

@carlos-rian
Copy link

@davidhewitt I updated uvicorn, uvloop and FastAPI to the latest version.

I was unable to reproduce the error.

@davidhewitt
Copy link
Member

Glad to hear! In the meantime I'll continue thinking on what (if anything) PyO3 can do differently here.

@davidhewitt
Copy link
Member

Returning to the original reproduction and what to do about it, I spent some further time thinking about this today. The main question which I wanted to answer is who's responsible for passing the in-flight exception around.

I think CPython is doing the correct thing to call PyO3's tp_dealloc which eventually calls the problematic Drop implementation. As a reminder, it looks like this:

impl Drop for SomeAwaitable {
    fn drop(&mut self) {
        let bt = backtrace::Backtrace::new();

        println!("Rust Backtrace:\n{:?}", bt);
        Python::with_gil(|py| {
            /// HERE: comment this out and the error goes away
            _ = py.run("a = 1", None, None);
        });
    }
}

Now, there's two possible arguments which could be made about who should be handling the in-flight exception:

  • it is the Drop implementation's responsibility to handle in-flight exceptions correctly
  • alternatively, PyO3 should protect against bad Drop implementations by saving any in-flight exception value before calling the drop and then restoring the exception afterwards

At the moment we leave all responsibility to the Drop implementation. I believe this is correct - most Drop implementations will be trivial so saving & restoring the exception state will be a useless deoptimization. Not tampering with the exception state inside PyO3 also leaves space for Drop implementations to interact with it, if needed.

I would be open to the possibility of adding a warning in debug builds where PyO3 checks that if there is an in-flight exception before calling Drop, there should be an in-flight exception after. This would identify the Drop implementation here as problematic during testing.

Does this mean it is impossible to use pyo3-log in a Drop call in such a circumstance?

I would like to update my answer to this question. You can use pyo3-log in a Drop call, however you probably need to save and restore any in-flight exception as part of your Drop implementation. More concretely, this is a correct Drop implementation which avoids the original reported crash:

impl Drop for SomeAwaitable {
    fn drop(&mut self) {
        let bt = backtrace::Backtrace::new();

        println!("Rust Backtrace:\n{:?}", bt);
        Python::with_gil(|py| {
            // save any existing exception
            let possible_error = PyErr::take(py);

            // do other stuff, maybe call pyo3-log etc.
            // NB rather than `let _` to discard any error, consider
            // using PyErr::write_unraisable to report it.
            let _ = py.run("a = 1", None, None);

            // restore any previous exception
            if let Some(possible_error) = possible_error {
                possible_error.restore(py);
            }
        });
    }
}

For now, I'm going to mark this issue as resolved, I've convinced myself there is no bug here in PyO3.

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

No branches or pull requests

3 participants