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

MemoryView implementation incompatible with Options.cache_builtins = False #3406

Closed
thirtythreeforty opened this issue Mar 10, 2020 · 3 comments · Fixed by #3415
Closed

MemoryView implementation incompatible with Options.cache_builtins = False #3406

thirtythreeforty opened this issue Mar 10, 2020 · 3 comments · Fixed by #3415
Labels
Milestone

Comments

@thirtythreeforty
Copy link

Disabling the cache_builtins option in setup.py:

Options.cache_builtins = False

causes code that uses MemoryView to fail to compile:

Error compiling Cython file:
------------------------------------------------------------
...
    if not is_slice:

        if start < 0:
            start += shape
        if not 0 <= start < shape:
            _err_dim(IndexError, "Index out of bounds (axis %d)", dim)
                    ^
------------------------------------------------------------

View.MemoryView:832:21: Accessing Python global or builtin not allowed without gil

(There are several such errors building the file, all stemming from doing a lookup on the exception type without the GIL held.)

To fix this, the GIL should be locked before calling _err_dim.

da-woods added a commit to da-woods/cython that referenced this issue Mar 11, 2020
Now they make their own tiny cache of the relevant exceptions
at module initialization so they can still access these without
the GIL.

closes cython#3406
@da-woods
Copy link
Collaborator

I've done this a tiny bit differently to how you've suggested, since I think the whole point of _err_dim was avoiding writing duplicate with gil: code but the basic diagnosis was good.

@thirtythreeforty
Copy link
Author

Great, thanks for the quick turnaround! Your way will prevent someone from mocking ValueError, but on the other hand I can't see anyone wanting to actually do that, so it should work!

@da-woods
Copy link
Collaborator

Your way will prevent someone from mocking ValueError, but on the other hand I can't see anyone wanting to actually do that, so it should work

I was just adding a comment to that effect in the PR description when you replied:

I guess this slightly breaks the contract of the cache_builtins option, that you should be able to monkey-patch builtin stuff (including exceptions). With that said - it's an implementation detail that it's written in Cython. If it was written in C then it'd just use the standard exception definitions in the headers and the user wouldn't be able to change them anyway.

I think the "implementation detail" excuse makes it OK but we'll see if it makes it past review...

scoder pushed a commit that referenced this issue Mar 24, 2020
Now they make their own tiny cache of the relevant exceptions at module initialization so they can still access these without the GIL.

closes #3406
@scoder scoder added this to the 3.0 milestone Mar 24, 2020
@scoder scoder added the defect label Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants