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

Error when using nogil in cpdef function that has nogil in signature #4137

Closed
vladima opened this issue Apr 26, 2021 · 4 comments · Fixed by #4318
Closed

Error when using nogil in cpdef function that has nogil in signature #4137

vladima opened this issue Apr 26, 2021 · 4 comments · Fixed by #4318

Comments

@vladima
Copy link
Contributor

vladima commented Apr 26, 2021

Code below used to compile in 0.29.21 but fails on master with error

cpdef void test(const int i) nogil:
with nogil, parallel(num_threads=i):
^
main.pyx:4:9: Trying to release the GIL while it was previously released.

from cython.parallel import parallel

cpdef void test(const int i) nogil:
    with nogil, parallel(num_threads=i):
        pass

The documentation for nogil states:

The function does not in itself release the GIL if it is held by the caller.

So the error looks like a bug.

@da-woods
Copy link
Contributor

da-woods commented Apr 27, 2021

That does look like a bug.

If you want to investigate, then git bisect is often useful for finding out why things have decided to stop working

@da-woods
Copy link
Contributor

Thinking about it more, I'm not sure if it's a bug. I think it depends on whether a with nogil block claims the GIL on exit, or if it restores the previous state. I don't know off the top my head.

If it claims the GIL on exit then the code was probably dodgy on Cython 0.29.

I'd it restores the previous state then there's no reason to make nested with gil blocks an error.

See also #3443

@scoder
Copy link
Contributor

scoder commented Apr 27, 2021 via email

@da-woods
Copy link
Contributor

da-woods commented Apr 27, 2021

It looks like with nogil expands to:

#ifdef WITH_THREAD
PyThreadState *_save;
Py_UNBLOCK_THREADS  /* i.e. _save = PyEval_SaveThread() */
__Pyx_FastGIL_Remember();
#endif

then

#ifdef WITH_THREAD
__Pyx_FastGIL_Forget();
Py_BLOCK_THREADS  /* PyEval_RestoreThread(_save); */
#endif

The documentation for PyEval_SaveThread says

If the lock has been created, the current thread must have acquired it.

and PyEval_RestoreThread says

Acquire the global interpreter lock (if it has been created)

Therefore, the with nogil block can only be used when you genuinely hold the GIL and therefore the code that used to "work" in Cython 0.29 was broken.

Personally, I think it might be worth changing this since it's quite hard to do the right thing in a function marked nogil (where we don't actual know whether it has the GIL or not). with gil uses PyGILState_Ensure/Release which does allow recursive calls so should be safer (but doesn't work with subinterpretters)

da-woods added a commit to da-woods/cython that referenced this issue Jul 28, 2021
Fixes cython#4137

Also adds a check whether we have the GIL before doing so. This
is important because Py_UNBLOCK_THREADS is documented as unsave
if we don't hold the GIL. This bit of it is possibly a candidate to be
backported (since the pattern was allowed in Cython 0.29 but
the behaviour looks like it could be quite buggy).
da-woods added a commit to da-woods/cython that referenced this issue Jul 28, 2021
Fixes cython#4137

Also adds a check whether we have the GIL before doing so. This
is important because Py_UNBLOCK_THREADS is documented as unsave
if we don't hold the GIL. This bit of it is possibly a candidate to be
backported (since the pattern was allowed in Cython 0.29 but
the behaviour looks like it could be quite buggy).
@scoder scoder added this to the 3.0 milestone Dec 18, 2021
scoder pushed a commit that referenced this issue Dec 18, 2021
Also adds a check whether we have the GIL before doing so. This
is important because Py_UNBLOCK_THREADS is documented as unsafe
if we don't hold the GIL.

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

Successfully merging a pull request may close this issue.

3 participants