Repository: cython/cython
Pull request: Fix issues with parallel/exception checked functions
Essentially the problem is that a `nogil` function just promises that the function *can* be called without the GIL, not that it actually doesn't have the GIL.

If you call `prange`/`parallel` inside that function while still holding the GIL, if any thread has finished the GIL will be held (... I think it's probably more of a mess than this, but mostly you get away with it...). If a thread is still running and tries to acquire the GIL then it'll deadlock permenantly.

I've solved this by adding an implicit `with nogil` around any `prange`/`parallel` that's in a `nogil` block but isn't certain that it has the GIL.

Fixes #5573 and #5564.

Problem statement:
[BUG] OpenMP does not work anymore with function call on cython 3.0
### Describe the bug

After upgrading from Cython 0.29.36 to Cython 3.0.0 produced code just hang infinitly when OpenMP is used and sub-function are called.

Looking at produced code for bellow lines:
```python
for j in prange(J):
    foo_vec(x[:, j], y[:, j])
```

Using Cython 0.29.36 produced code is:
```c
// for j in prange(J):

  __pyx_t_1 = __pyx_v_J;
  if ((1 == 0)) abort();
  {
      #if ((defined(__APPLE__) || defined(__OSX__)) && (defined(__GNUC__) && (__GNUC__ > 2 || (__GNUC__ == 2 && (__GNUC_MINOR__ > 95)))))
          #undef likely
          #undef unlikely
          #define likely(x)   (x)
          #define unlikely(x) (x)
      #endif
      __pyx_t_3 = (__pyx_t_1 - 0 + 1 - 1/abs(1)) / 1;
      if (__pyx_t_3 > 0)
      {
          #ifdef _OPENMP
          #pragma omp parallel
          #endif /* _OPENMP */
          {
              #ifdef _OPENMP
              #pragma omp for firstprivate(__pyx_v_j) lastprivate(__pyx_v_j)
              #endif /* _OPENMP */
              for (__pyx_t_2 = 0; __pyx_t_2 < __pyx_t_3; __pyx_t_2++){
                  {
                      __pyx_v_j = (Py_ssize_t)(0 + 1 * __pyx_t_2);

//     foo_vec(x[:, j], y[:, j])

                      __pyx_t_4.data = __pyx_v_x.data;
                      __pyx_t_4.memview = __pyx_v_x.memview;
                      __PYX_INC_MEMVIEW(&__pyx_t_4, 0);
                      __pyx_t_4.shape[0] = __pyx_v_x.shape[0];
__pyx_t_4.strides[0] = __pyx_v_x.strides[0];
    __pyx_t_4.suboffsets[0] = -1;

{
    Py_ssize_t __pyx_tmp_idx = __pyx_v_j;
    Py_ssize_t __pyx_tmp_stride = __pyx_v_x.strides[1];
        __pyx_t_4.data += __pyx_tmp_idx * __pyx_tmp_stride;
}

__pyx_t_5.data = __pyx_v_y.data;
                      __pyx_t_5.memview = __pyx_v_y.memview;
                      __PYX_INC_MEMVIEW(&__pyx_t_5, 0);
                      __pyx_t_5.shape[0] = __pyx_v_y.shape[0];
__pyx_t_5.strides[0] = __pyx_v_y.strides[0];
    __pyx_t_5.suboffsets[0] = -1;

{
    Py_ssize_t __pyx_tmp_idx = __pyx_v_j;
    Py_ssize_t __pyx_tmp_stride = __pyx_v_y.strides[1];
        __pyx_t_5.data += __pyx_tmp_idx * __pyx_tmp_stride;
}

__pyx_f_6my_ext_foo_vec(__pyx_t_4, __pyx_t_5);
                      __PYX_XDEC_MEMVIEW(&__pyx_t_4, 0);
                      __pyx_t_4.memview = NULL;
                      __pyx_t_4.data = NULL;
                      __PYX_XDEC_MEMVIEW(&__pyx_t_5, 0);
                      __pyx_t_5.memview = NULL;
                      __pyx_t_5.data = NULL;
                  }
              }
          }
      }
  }
  #if ((defined(__APPLE__) || defined(__OSX__)) && (defined(__GNUC__) && (__GNUC__ > 2 || (__GNUC__ == 2 && (__GNUC_MINOR__ > 95)))))
      #undef likely
      #undef unlikely
      #define likely(x)   __builtin_expect(!!(x), 1)
      #define unlikely(x) __builtin_expect(!!(x), 0)
  #endif
```

Using Cython 3.0 produced code is:
```c
// for j in prange(J):

  __pyx_t_1 = __pyx_v_J;
  {
      #if ((defined(__APPLE__) || defined(__OSX__)) && (defined(__GNUC__) && (__GNUC__ > 2 || (__GNUC__ == 2 && (__GNUC_MINOR__ > 95)))))
          #undef likely
          #undef unlikely
          #define likely(x)   (x)
          #define unlikely(x) (x)
      #endif
      __pyx_t_3 = (__pyx_t_1 - 0 + 1 - 1/abs(1)) / 1;
      if (__pyx_t_3 > 0)
      {
          #ifdef _OPENMP
          #pragma omp parallel
          #endif /* _OPENMP */
          {
              #ifdef _OPENMP
              #pragma omp for firstprivate(__pyx_v_j) lastprivate(__pyx_v_j)
              #endif /* _OPENMP */
              for (__pyx_t_2 = 0; __pyx_t_2 < __pyx_t_3; __pyx_t_2++){
                  {
                      __pyx_v_j = (Py_ssize_t)(0 + 1 * __pyx_t_2);

//    foo_vec(x[:, j], y[:, j])

                      __pyx_t_4.data = __pyx_v_x.data;
                      __pyx_t_4.memview = __pyx_v_x.memview;
                      __PYX_INC_MEMVIEW(&__pyx_t_4, 0);
                      __pyx_t_4.shape[0] = __pyx_v_x.shape[0];
__pyx_t_4.strides[0] = __pyx_v_x.strides[0];
    __pyx_t_4.suboffsets[0] = -1;

{
    Py_ssize_t __pyx_tmp_idx = __pyx_v_j;
    Py_ssize_t __pyx_tmp_stride = __pyx_v_x.strides[1];
        __pyx_t_4.data += __pyx_tmp_idx * __pyx_tmp_stride;
}

__pyx_t_5.data = __pyx_v_y.data;
                      __pyx_t_5.memview = __pyx_v_y.memview;
                      __PYX_INC_MEMVIEW(&__pyx_t_5, 0);
                      __pyx_t_5.shape[0] = __pyx_v_y.shape[0];
__pyx_t_5.strides[0] = __pyx_v_y.strides[0];
    __pyx_t_5.suboffsets[0] = -1;

{
    Py_ssize_t __pyx_tmp_idx = __pyx_v_j;
    Py_ssize_t __pyx_tmp_stride = __pyx_v_y.strides[1];
        __pyx_t_5.data += __pyx_tmp_idx * __pyx_tmp_stride;
}

__pyx_f_6my_ext_foo_vec(__pyx_t_4, __pyx_t_5); if (unlikely(__Pyx_ErrOccurredWithGIL())) __PYX_ERR(0, 40, __pyx_L5_error)
                      __PYX_XCLEAR_MEMVIEW(&__pyx_t_4, 0);
                      __pyx_t_4.memview = NULL; __pyx_t_4.data = NULL;
                      __PYX_XCLEAR_MEMVIEW(&__pyx_t_5, 0);
                      __pyx_t_5.memview = NULL; __pyx_t_5.data = NULL;
                      goto __pyx_L8;
                      __pyx_L5_error:;
                      {
                          #ifdef WITH_THREAD
                          PyGILState_STATE __pyx_gilstate_save = __Pyx_PyGILState_Ensure();
                          #endif
                          #ifdef _OPENMP
                          #pragma omp flush(__pyx_parallel_exc_type)
                          #endif /* _OPENMP */
                          if (!__pyx_parallel_exc_type) {
                            __Pyx_ErrFetchWithState(&__pyx_parallel_exc_type, &__pyx_parallel_exc_value, &__pyx_parallel_exc_tb);
                            __pyx_parallel_filename = __pyx_filename; __pyx_parallel_lineno = __pyx_lineno; __pyx_parallel_clineno = __pyx_clineno;
                            __Pyx_GOTREF(__pyx_parallel_exc_type);
                          }
                          #ifdef WITH_THREAD
                          __Pyx_PyGILState_Release(__pyx_gilstate_save);
                          #endif
                      }
                      __pyx_parallel_why = 4;
                      goto __pyx_L7;
                      __pyx_L7:;
                      #ifdef _OPENMP
                      #pragma omp critical(__pyx_parallel_lastprivates0)
                      #endif /* _OPENMP */
                      {
                          __pyx_parallel_temp0 = __pyx_v_j;
                      }
                      __pyx_L8:;
                      #ifdef _OPENMP
                      #pragma omp flush(__pyx_parallel_why)
                      #endif /* _OPENMP */
                  }
              }
              #ifdef _OPENMP
              Py_END_ALLOW_THREADS
              #else
{
#ifdef WITH_THREAD
              PyGILState_STATE __pyx_gilstate_save = __Pyx_PyGILState_Ensure();
              #endif
              #endif /* _OPENMP */
              /* Clean up any temporaries */
              __PYX_XCLEAR_MEMVIEW(&__pyx_t_4, 0);
              __pyx_t_4.memview = NULL; __pyx_t_4.data = NULL;
              __PYX_XCLEAR_MEMVIEW(&__pyx_t_5, 0);
              __pyx_t_5.memview = NULL; __pyx_t_5.data = NULL;
              #ifdef WITH_THREAD
              __Pyx_PyGILState_Release(__pyx_gilstate_save);
              #endif
              #ifndef _OPENMP
}
#endif /* _OPENMP */
          }
      }
      if (__pyx_parallel_exc_type) {
        /* This may have been overridden by a continue, break or return in another thread. Prefer the error. */
        __pyx_parallel_why = 4;
      }
      if (__pyx_parallel_why) {
        __pyx_v_j = __pyx_parallel_temp0;
        switch (__pyx_parallel_why) {
              case 4:
          {
              #ifdef WITH_THREAD
              PyGILState_STATE __pyx_gilstate_save = __Pyx_PyGILState_Ensure();
              #endif
              __Pyx_GIVEREF(__pyx_parallel_exc_type);
              __Pyx_ErrRestoreWithState(__pyx_parallel_exc_type, __pyx_parallel_exc_value, __pyx_parallel_exc_tb);
              __pyx_filename = __pyx_parallel_filename; __pyx_lineno = __pyx_parallel_lineno; __pyx_clineno = __pyx_parallel_clineno;
              #ifdef WITH_THREAD
              __Pyx_PyGILState_Release(__pyx_gilstate_save);
              #endif
          }
          goto __pyx_L1_error;
        }
      }
  }
  #if ((defined(__APPLE__) || defined(__OSX__)) && (defined(__GNUC__) && (__GNUC__ > 2 || (__GNUC__ == 2 && (__GNUC_MINOR__ > 95)))))
      #undef likely
      #undef unlikely
      #define likely(x)   __builtin_expect(!!(x), 1)
      #define unlikely(x) __builtin_expect(!!(x), 0)
  #endif
```

I am not expecting any GIL check / any other check in `prange` body.

### Code to reproduce the behaviour:

my_ext.pyx :

```cython
cimport cython
from cython.parallel cimport prange

cimport numpy as np

np.import_array()


def foo(np.ndarray x not None):
    cdef int ndim = np.PyArray_NDIM(x)
    y = np.PyArray_EMPTY(ndim, np.PyArray_DIMS(x), np.NPY_FLOAT64, 0)

    if ndim == 1:
        foo_vec(x, y)
    elif ndim == 2:
        foo_mat(x, y)

    return y


@cython.boundscheck(False)
@cython.wraparound(False)
cdef void foo_vec(const double[:] x, double[:] y) nogil:
    cdef:
        Py_ssize_t I = x.shape[0]
        Py_ssize_t i

    for i in range(I):
        y[i] = x[i] * 2.0 + 1.0  # Do whatever computation on vec


@cython.boundscheck(False)
@cython.wraparound(False)
cdef void foo_mat(const double[:, :] x,  double[:, :] y) nogil:
    cdef:
        Py_ssize_t J = x.shape[1]
        Py_ssize_t j
    
    for j in prange(J):
        foo_vec(x[:, j], y[:, j])

```

setup.py :

```python
from setuptools import setup, Extension
from Cython.Build import cythonize
import numpy as np

setup(
    ext_modules=cythonize(
        [Extension(
            "my_ext", ["my_ext.pyx"],
            include_dirs=[np.get_include()],
            extra_compile_args=["-fopenmp"],
            extra_link_args=["-fopenmp"],
            define_macros=[("NPY_NO_DEPRECATED_API", "NPY_1_7_API_VERSION")],
        )],
        language_level=3,
        annotate=True,
    )
)
```


### Expected behaviour

I am expecting code produced code that:
- does not require any GIL check 
- works correctly on latest Cython version😀 

### OS

Linux

### Python version

3.10.6

### Cython version

3.0.0

### Additional context

_No response_

Hints from issue discussion:
I finaly found that the `noexcept` is now required before `nogil` qualifier to tell cython that `cdef` function will not raise any exception.

It fix this issue.
@arthurlm This looks like a simpler duplicate of https://github.com/cython/cython/issues/5564. So thanks for the example - it'll hopefully be easier to investigate

It shouldn't be hanging without `noexcept` - this is a bug that I hope to fix in the near future. Although `noexcept` may well give a speed improvement.
~Slightly alarmingly, I now don't understand how this ever worked....~

~As far as I can tell we release the GIL inside the omp parallel block (so we release the GIL for *every* thread), and we reacquire the GIL inside the the omp parallel block (as at the end we require *every* thread to hold the GIL).~

This isn't true I think... ignore
Got to the bottom of this.

I believe there's a subtle error in your code: a` nogil` function says that it can be run without the GIL, but doesn't guarantee that it's run without the GIL. `prange` blocks must be run without the GIL (although you get away with it most of the time unless you hit something that needs to re-acquire the GIL). In your case you don't release the GIL before calling `foo_mat`, so one thread holds the GIL for the whole time.

Frankly that's so subtle that I'm not sure anyone can be expected to spot it. Therefore my proposed fix is to get rid of the requirement and make absolutely 100% sure that the GIL is released before entering a `prange` block.

Commit message: Add extra field in pxd

Patch to review:
diff --git a/Cython/Compiler/ExprNodes.py b/Cython/Compiler/ExprNodes.py
index 275a6233da5..02799816640 100644
--- a/Cython/Compiler/ExprNodes.py
+++ b/Cython/Compiler/ExprNodes.py
@@ -6291,6 +6291,12 @@ def analyse_c_function_call(self, env):
             if needs_cpp_exception_conversion(func_type):
                 env.use_utility_code(UtilityCode.load_cached("CppExceptionConversion", "CppSupport.cpp"))
 
+        if (func_type.exception_value is not None or
+                func_type.exception_check and func_type.exception_check != '+'):
+            if env.nogil:
+                # need to generate refnanny for testing because of exception check
+                env.has_with_gil_block = True
+
         self.overflowcheck = env.directives['overflowcheck']
 
     def calculate_result_code(self):
diff --git a/Cython/Compiler/ParseTreeTransforms.pxd b/Cython/Compiler/ParseTreeTransforms.pxd
index efbb14f70ba..b44c290a1a8 100644
--- a/Cython/Compiler/ParseTreeTransforms.pxd
+++ b/Cython/Compiler/ParseTreeTransforms.pxd
@@ -76,6 +76,7 @@ cdef class GilCheck(VisitorTransform):
     cdef list env_stack
     cdef bint nogil
     cdef bint nogil_declarator_only
+    cdef bint current_gilstat_node_knows_gil_state
 
 cdef class TransformBuiltinMethods(EnvTransform):
     cdef visit_cython_attribute(self, node)
diff --git a/Cython/Compiler/ParseTreeTransforms.py b/Cython/Compiler/ParseTreeTransforms.py
index b5c6f982473..92939e7ccf7 100644
--- a/Cython/Compiler/ParseTreeTransforms.py
+++ b/Cython/Compiler/ParseTreeTransforms.py
@@ -3514,6 +3514,7 @@ def __call__(self, root):
         # True for 'cdef func() nogil:' functions, as the GIL may be held while
         # calling this function (thus contained 'nogil' blocks may be valid).
         self.nogil_declarator_only = False
+        self.current_gilstat_node_knows_gil_state = False
         return super(GilCheck, self).__call__(root)
 
     def _visit_scoped_children(self, node, gil_state):
@@ -3575,13 +3576,23 @@ def visit_GILStatNode(self, node):
             # which is wrapped in a StatListNode. Just unpack that.
             node.finally_clause, = node.finally_clause.stats
 
+        nogil_declarator_only = self.nogil_declarator_only
+        self.nogil_declarator_only = False
+        current_gilstat_node_knows_gil_state = self.current_gilstat_node_knows_gil_state
+        self.current_gilstat_node_knows_gil_state = node.scope_gil_state_known
         self._visit_scoped_children(node, is_nogil)
+        self.nogil_declarator_only = nogil_declarator_only
+        self.current_gilstat_node_knows_gil_state = current_gilstat_node_knows_gil_state
         return node
 
     def visit_ParallelRangeNode(self, node):
-        if node.nogil:
-            node.nogil = False
+        if node.nogil or self.nogil_declarator_only:
+            node_was_nogil, node.nogil = node.nogil, False
             node = Nodes.GILStatNode(node.pos, state='nogil', body=node)
+            if not node_was_nogil and self.nogil_declarator_only:
+                # We're in a "nogil" function, but that doesn't prove we
+                # didn't have the gil
+                node.scope_gil_state_known = False
             return self.visit_GILStatNode(node)
 
         if not self.nogil:
@@ -3598,6 +3609,12 @@ def visit_ParallelWithBlockNode(self, node):
             error(node.pos, "The parallel section may only be used without "
                             "the GIL")
             return None
+        if self.nogil_declarator_only:
+            # We're in a "nogil" function but that doesn't prove we didn't
+            # have the gil, so release it
+            node = Nodes.GILStatNode(node.pos, state='nogil', body=node)
+            node.scope_gil_state_known = False
+            return self.visit_GILStatNode(node)
 
         if node.nogil_check:
             # It does not currently implement this, but test for it anyway to
@@ -3620,7 +3637,7 @@ def visit_TryFinallyStatNode(self, node):
         return node
 
     def visit_GILExitNode(self, node):
-        if self.nogil_declarator_only:
+        if not self.current_gilstat_node_knows_gil_state:
             node.scope_gil_state_known = False
         self.visitchildren(node)
         return node
diff --git a/tests/run/parallel.pyx b/tests/run/parallel.pyx
index 40d7ac10de9..44a3a38f2dc 100644
--- a/tests/run/parallel.pyx
+++ b/tests/run/parallel.pyx
@@ -98,5 +98,40 @@ def test_parallel_catch():
 '''
 
 
+cdef void parallel_exception_checked_function(int* ptr, int id) except * nogil:
+    # requires the GIL after each call
+    ptr[0] = id;
+
+cdef void parallel_call_exception_checked_function_impl(int* arr, int num_threads) nogil:
+    # Inside a nogil function, parallel can't be sure that the GIL has been released.
+    # Therefore Cython must release the GIL itself.
+    # Otherwise, we can experience cause lock-ups if anything inside it acquires the GIL
+    # (since if any other thread has finished, it will be holding the GIL).
+    #
+    # An equivalent test with prange is in "sequential_parallel.pyx"
+    with cython.parallel.parallel(num_threads=num_threads):
+        parallel_exception_checked_function(arr+threadid(), threadid())
+
+
+def test_parallel_call_exception_checked_function():
+    """
+    test_parallel_call_exception_checked_function()
+    """
+    cdef int maxthreads = openmp.omp_get_max_threads()
+    cdef int *buf = <int *> malloc(sizeof(int) * maxthreads)
+
+    if buf == NULL:
+        raise MemoryError
+
+    try:
+        # Note we *don't* release the GIL here
+        parallel_call_exception_checked_function_impl(buf, maxthreads)
+
+        for i in range(maxthreads):
+            assert buf[i] == i
+    finally:
+        free(buf)
+
+
 OPENMP_PARALLEL = True
 include "sequential_parallel.pyx"
diff --git a/tests/run/sequential_parallel.pyx b/tests/run/sequential_parallel.pyx
index cd4bbd6bc27..5fe4a737b67 100644
--- a/tests/run/sequential_parallel.pyx
+++ b/tests/run/sequential_parallel.pyx
@@ -816,3 +816,36 @@ def test_inner_private():
     assert inner_are_the_same == False,  "Temporary variables in inner loop should be private"
 
     print('ok')
+
+cdef void prange_exception_checked_function(int* ptr, int id) except * nogil:
+    # requires the GIL after each call
+    ptr[0] = id;
+
+cdef void prange_call_exception_checked_function_impl(int* arr, int N) nogil:
+    # Inside a nogil function, prange can't be sure the GIL has been released.
+    # Therefore Cython must release the GIL itself.
+    # Otherwise, we can experience cause lock-ups if anything inside it acquires the GIL
+    # (since if any other thread has finished, it will be holding the GIL).
+    #
+    # An equivalent test with prange is in "sequential_parallel.pyx"
+    cdef int i
+    for i in prange(N, num_threads=4, schedule='static', chunksize=1):
+        prange_exception_checked_function(arr+i, i)
+
+def test_prange_call_exception_checked_function():
+    """
+    >>> test_prange_call_exception_checked_function()
+    """
+
+    cdef int N = 10000
+    cdef int* buf = <int*>malloc(sizeof(int)*N)
+    if buf == NULL:
+        raise MemoryError
+    try:
+        # Don't release the GIL
+        prange_call_exception_checked_function_impl(buf, N)
+
+        for i in range(N):
+            assert buf[i] == i
+    finally:
+        free(buf)


Code review comments:
diff_hunk: @@ -3514,6 +3514,7 @@ def __call__(self, root):
         # True for 'cdef func() nogil:' functions, as the GIL may be held while
         # calling this function (thus contained 'nogil' blocks may be valid).
         self.nogil_declarator_only = False
+        self.current_gilstat_node_knows_gil_state = False
path: Cython/Compiler/ParseTreeTransforms.py
text: @user1:
The comment above doesn't apply to this attribute any more, so better separate the two visually.
```suggestion

        self.current_gilstat_node_knows_gil_state = False
```