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

fix regression discussed by @cyfdecyf in #1454 #1517

Merged
merged 1 commit into from
Sep 11, 2018
Merged

Conversation

wjakob
Copy link
Member

@wjakob wjakob commented Sep 9, 2018

The PYBIND11_TLS_DELETE_VALUE in PR #1454 changed behavior wrt. Python 2.x. This commit restores the old behavior.

@wjakob
Copy link
Member Author

wjakob commented Sep 9, 2018

@cyfdecyf: I was not able to reproduce the issue when calling pure virtual functions multiple times in Python 2.7.x, thus it's hard for me to say if this PR addresses the issue. Could you please give it a try? For the record, it would also be great to know what OS you're running this on.

@cyfdecyf
Copy link

cyfdecyf commented Sep 11, 2018

Here's a simple test case. I can confirm this commit fixes the problem.

Note the problem only occurs when gil_scope_acquire is called in a non-python created thread, thus only test_in_new_thread will trigger the bug.

Without this fix, test.py will segfault in gil_scope_acquire in the second lock because it's getting the already deleted TLS tstate which is allocated in the 1st call.

Environment:
Ubuntu 16.04.4
Python 2.7.12

@wjakob
Copy link
Member Author

wjakob commented Sep 11, 2018

Thank you @cyfdecyf, I added your test.

@wjakob wjakob merged commit 44e39e0 into pybind:master Sep 11, 2018
wjakob added a commit that referenced this pull request Sep 11, 2018
facebook-github-bot pushed a commit to pytorch/pytorch that referenced this pull request Sep 11, 2018
Summary:
Fixes #11419

In particular pulling in pybind/pybind11#1454
as well as pending bugfix in pybind/pybind11#1517 (documenting in comment)
Pull Request resolved: #11473

Differential Revision: D9776003

Pulled By: jamesr66a

fbshipit-source-id: a225dcfb66c06bcae98fd2508d9e690c24be551a
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

Successfully merging this pull request may close these issues.

None yet

2 participants