• Matěj Laitl's avatar
    Fix calling an "except +" cpp function in a nogil function · 6795a2ba
    Matěj Laitl authored
    For a source:
    |cdef extern from "<foo>":
    |    cdef void foo_cpp() nogil except +
    |
    |cdef call_foo() nogil:
    |    foo_cpp()
    
    We used to generate something like this to actually call foo_cpp() in call_foo():
    |try{foo_cpp();}
    |catch(...) {
    |  Py_BLOCK_THREADS; __Pyx_CppExn2PyErr(); Py_UNBLOCK_THREADS
    |  `code.error_goto(self.pos))`
    |}
    
    where Py_BLOCK_THREADS expands to "PyEval_RestoreThread(_save);".
    __Pyx_CppExn2PyErr() (and alternatives, see SimpleCallNode) calls CPython A API
    methods so it needs to be guarded in a nogil environment.
    
    One problem is that "PyThreadState *_save" is only declared by "with nogil:"
    block, so a .cpp file that doesn't compile is generated for the above code.
    
    However, I think the real issue is that Py_(UN)BLOCK_THREADS is inappropriate
    here, as it isn't allowed to be called recursively and is valid only directly
    in a Py_BEGIN_ALLOW_THREADS ... Py_END_ALLOW_THREADS. IMO PyGILState_Ensure()
    and PyGILState_Release() (through `code.put_ensure_gil()` and a friend) is the
    correct thing to call here as it is allowed to be called recursively and
    actually ensures the current thread can call CPython C API.
    
    This patch does exactly this (and it breaks the generated code to multiple
    lines as it would be way too long otherwise), plus it extends the
    cpp_exceptions_nogil.pyx test with above example that doesn't compile with this
    fix not applied.
    
    Note that we explicitly pass declare_gilstate=True to put_ensure_gil(), as
    PyGILState_Ensure() docs state that recursive calls to it must not share the
    PyGILState_STATE. C++-style declaring a variable inside a block should be
    no-problem here, as try{} .. catch() is obviously valid only in a C++ code.
    6795a2ba
cpp_exceptions_nogil.pyx 5.21 KB