Skip to content

PyGILState_Ensure on non-Python thread causes fatal error #65090

New issue
New issue
Closed
Closed
PyGILState_Ensure on non-Python thread causes fatal error#65090
Labels
3.7 (EOL)end of lifeextension-modulesC modules in the Modules dir

Description

@zooba
zooba
opened on Mar 11, 2014
BPO 20891
Nosy @gvanrossum, @warsaw, @ncoghlan, @pitrou, @vstinner, @ericsnowcurrently, @zooba
PRs
  • bpo-20891: Fix PyGILState_Ensure() #4650
  • [3.6] bpo-20891: Fix PyGILState_Ensure() (#4650) #4655
  • [2.7] bpo-20891: Fix PyGILState_Ensure() (#4650) #4657
  • bpo-20891: Py_Initialize() now creates the GIL #4700
  • bpo-20891: Skip test_embed.test_bpo20891() #4967
  • [3.6] bpo-20891: Skip test_embed.test_bpo20891() (#4967) #4969
  • bpo-20891: Reenable test_embed.test_bpo20891() #5420
  • [3.6] bpo-20891: Py_Initialize() now creates the GIL (#4700) #5421
  • [3.6] bpo-20891: Remove test_capi.test_bpo20891() #5425
    		•
    Files
  • test.c
  • ptest.c
  • PyGILState_Ensure.patch
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = <Date 2018-01-29.13:07:27.074>
created_at = <Date 2014-03-11.18:40:13.148>
labels = ['extension-modules', '3.7']
title = 'PyGILState_Ensure on non-Python thread causes fatal error'
updated_at = <Date 2018-01-29.15:35:52.778>
user = 'https://github.com/zooba'

    bugs.python.org fields:

    activity = <Date 2018-01-29.15:35:52.778>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2018-01-29.13:07:27.074>
closer = 'vstinner'
components = ['Extension Modules']
creation = <Date 2014-03-11.18:40:13.148>
creator = 'steve.dower'
dependencies = []
files = ['34358', '42173', '42174']
hgrepos = []
issue_num = 20891
keywords = ['patch']
message_count = 31.0
messages = ['213160', '213161', '213398', '261820', '261821', '307038', '307330', '307331', '307342', '307349', '307350', '307351', '307550', '307551', '307552', '307553', '307554', '307571', '307583', '307586', '307628', '308588', '308914', '308917', '311090', '311091', '311095', '311099', '311124', '311125', '311143']
nosy_count = 10.0
nosy_names = ['gvanrossum', 'barry', 'ncoghlan', 'pitrou', 'vstinner', 'Mekk', 'kchen', 'eric.snow', 'steve.dower', 'xcombelle']
pr_nums = ['4650', '4655', '4657', '4700', '4967', '4969', '5420', '5421', '5425']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue20891'
versions = ['Python 2.7', 'Python 3.6', 'Python 3.7']

    Activity

    1. zooba commented on Mar 11, 2014

      @zooba
      zooba
      on Mar 11, 2014
      MemberAuthor

      In Python 3.4rc3, calling PyGILState_Ensure() from a thread that was not created by Python and without any calls to PyEval_InitThreads() will cause a fatal exit:

      Fatal Python error: take_gil: NULL tstate

      I believe this was added in http://hg.python.org/cpython/rev/dc4e805ec68a. The attached file has a minimal repro (for Windows, sorry, but someone familiar with low-level *nix threading APIs should be able to translate it easily).

      Basically, the sequence looks like this:

      1. main() calls Py_Initialize()
      2. main() starts new C-level thread
      3. main() calls Py_BEGIN_ALLOW_THREADS and waits for the new thread
      4. New thread calls PyGILState_Ensure()
      5. PyGILState_Ensure() sees that there is no thread state and calls PyEval_InitThreads() (this is what changed in the linked changeset)
      6. PyEval_InitThreads() creates the GIL
      7. PyEval_InitThreads() calls take_gil(PyThreadState_GET()) (this is what aborts - there is still no thread state)
      8. (expected) PyGILState_Ensure() calls PyThreadState_New() and PyEval_RestoreThread().

      The attached repro has two workarounds (commented out). The first is to call PyThreadState_New()/PyEval_RestoreThread() from the new thread, and the second is to call PyEval_InitThreads() from the main thread.

      The latter is inappropriate for my use, since I am writing a debugger that can inject itself into a running CPython process - there is no way to require that PyEval_InitThreads() has been called and I can't reliably inject it onto the main thread. The first workaround is okay, but I'd rather not have to special-case 3.4 by rewriting PyGILState_Ensure() so that I can safely call PyGILState_Ensure().

    2. zooba
      added
      extension-modulesC modules in the Modules dir
      on Mar 11, 2014

    3. zooba commented on Mar 11, 2014

      @zooba
      zooba
      on Mar 11, 2014
      MemberAuthor

      Should have linked to bpo-19576 as well, which is the issue associated with that changeset.

    4. vstinner commented on Mar 13, 2014

      @vstinner
      vstinner
      on Mar 13, 2014
      Member

      IMO it's a bug in PyEval_InitThreads().

    5. vstinner commented on Mar 15, 2016

      @vstinner
      vstinner
      on Mar 15, 2016
      Member

      ptest.c: Portable example using Python API.

    6. vstinner commented on Mar 15, 2016

      @vstinner
      vstinner
      on Mar 15, 2016
      Member

      Attached patch should fix the issue.

    7. Mekk commented on Nov 27, 2017

      @Mekk
      Mekkmannequin
      on Nov 27, 2017
      Mannequin

      Is this fix released? I can't find it in the changelog…

      (I faced this bug on 3.5.2, released a couple of months after this bug was closed…)

    8. vstinner commented on Nov 30, 2017

      @vstinner
      vstinner
      on Nov 30, 2017
      Member

      Is this fix released? I can't find it in the changelog…

      Oops, I lost track of this issue, but it wasn't fixed, no.

      I just proposed my old fix as a pull requets: PR 4650.

      (I faced this bug on 3.5.2, released a couple of months after this bug was closed…)

      The workaround is to call PyEval_InitThreads() before spawning your first non-Python thread.
      https://docs.python.org/dev/c-api/init.html#c.PyEval_InitThreads

    9. vstinner commented on Nov 30, 2017

      @vstinner
      vstinner
      on Nov 30, 2017
      Member

      I have to check if Python 2.7 is impacted as well.

    10. vstinner
      added
      3.7 (EOL)end of life
      on Nov 30, 2017

    11. vstinner commented on Nov 30, 2017

      @vstinner
      vstinner
      on Nov 30, 2017
      Member

      New changeset b4d1e1f by Victor Stinner in branch 'master':
      bpo-20891: Fix PyGILState_Ensure() (bpo-4650)
      b4d1e1f

    12. vstinner commented on Nov 30, 2017

      @vstinner
      vstinner
      on Nov 30, 2017
      Member

      New changeset be6b74c by Victor Stinner in branch '2.7':
      bpo-20891: Fix PyGILState_Ensure() (bpo-4650) (bpo-4657)
      be6b74c

    13. vstinner commented on Nov 30, 2017

      @vstinner
      vstinner
      on Nov 30, 2017
      Member

      New changeset e10c9de by Victor Stinner in branch '3.6':
      bpo-20891: Fix PyGILState_Ensure() (bpo-4650) (bpo-4655)
      e10c9de

    14. vstinner commented on Nov 30, 2017

      @vstinner
      vstinner
      on Nov 30, 2017
      Member

      Ok, the bug is now fixed in Python 2.7, 3.6 and master (future 3.7). On 3.6 and master, the fix comes with an unit test.

      Thanks Steve Dower for the bug report, sorry for the long delay, I completely forgot this old bug!

      Marcin Kasperski: Thanks for the reminder. Sadly, we will have to wait for the next release to get the fix. In the meanwhile, you can workaround the bug by calling PyEval_InitThreads() after Py_Initialize() but before running actual code.

    15. vstinner
      closed this as completedon Nov 30, 2017

    16. 7 remaining items

    17. vstinner commented on Dec 4, 2017

      @vstinner
      vstinner
      on Dec 4, 2017
      Member

      I wrote the PR 4700 to create the GIL in Py_Initialize().

      Would you be ok to backport this fix to Python 2.7 and 3.6 as well?

    18. gvanrossum commented on Dec 4, 2017

      @gvanrossum
      gvanrossum
      on Dec 4, 2017
      Member

      For 2.7 I hesitate to OK this, who knows what skeletons are still in that
      closet. It doesn't sound like a security fix to me. For 3.6 I'm fine with
      it as a bugfix.

    19. ncoghlan commented on Dec 5, 2017

      @ncoghlan
      ncoghlan
      on Dec 5, 2017
      Contributor

      +1 for making this change 3.6+ only.

      Victor, could you run your patch through the performance benchmarks? While I suspect Antoine is right that our current GIL management heuristics will mean we don't need the lazy initialisation optimisation any more, it's still preferable to have the specific numbers before merging it.

    20. vstinner commented on Dec 18, 2017

      @vstinner
      vstinner
      on Dec 18, 2017
      Member

      I ran pyperformance on my PR 4700. Differences of at least 5%:

      haypo@speed-python$ python3 -m perf compare_to ~/json/uploaded/2017-12-18_12-29-master-bd6ec4d79e85.json.gz /home/haypo/json/patch/2017-12-18_12-29-master-bd6ec4d79e85-patch-4700.json.gz --table --min-speed=5

      +----------------------+--------------------------------------+-------------------------------------------------+
      | Benchmark | 2017-12-18_12-29-master-bd6ec4d79e85 | 2017-12-18_12-29-master-bd6ec4d79e85-patch-4700 |
      +======================+======================================+=================================================+
      | pathlib | 41.8 ms | 44.3 ms: 1.06x slower (+6%) |
      +----------------------+--------------------------------------+-------------------------------------------------+
      | scimark_monte_carlo | 197 ms | 210 ms: 1.07x slower (+7%) |
      +----------------------+--------------------------------------+-------------------------------------------------+
      | spectral_norm | 243 ms | 269 ms: 1.11x slower (+11%) |
      +----------------------+--------------------------------------+-------------------------------------------------+
      | sqlite_synth | 7.30 us | 8.13 us: 1.11x slower (+11%) |
      +----------------------+--------------------------------------+-------------------------------------------------+
      | unpickle_pure_python | 707 us | 796 us: 1.13x slower (+13%) |
      +----------------------+--------------------------------------+-------------------------------------------------+

      Not significant (55): 2to3; chameleon; chaos; (...)

    21. vstinner commented on Dec 21, 2017

      @vstinner
      vstinner
      on Dec 21, 2017
      Member

      New changeset 550ee05 by Victor Stinner in branch 'master':
      bpo-20891: Skip test_embed.test_bpo20891() (bpo-4967)
      550ee05

    22. vstinner commented on Dec 21, 2017

      @vstinner
      vstinner
      on Dec 21, 2017
      Member

      New changeset 2e1ef00 by Victor Stinner in branch '3.6':
      bpo-20891: Skip test_embed.test_bpo20891() (bpo-4967) (bpo-4969)
      2e1ef00

    23. vstinner commented on Jan 29, 2018

      @vstinner
      vstinner
      on Jan 29, 2018
      Member

      I ran pyperformance on my PR 4700. Differences of at least 5%: (...)

      I tested again these 5 benchmarks were Python was slower with my PR. I ran these benchmarks manually on my laptop using CPU isolation. Result:

      vstinner@apu$ python3 -m perf compare_to ref.json patch.json --table
      Not significant (5): unpickle_pure_python; sqlite_synth; spectral_norm; pathlib; scimark_monte_carlo

      Ok, that was expected: no significant difference.

    24. vstinner commented on Jan 29, 2018

      @vstinner
      vstinner
      on Jan 29, 2018
      Member

      New changeset 2914bb3 by Victor Stinner in branch 'master':
      bpo-20891: Py_Initialize() now creates the GIL (bpo-4700)
      2914bb3

    25. vstinner commented on Jan 29, 2018

      @vstinner
      vstinner
      on Jan 29, 2018
      Member

      I proposed PR 5421 to fix Python 3.6: modify Py_Initialize() to call PyEval_InitThreads().

      I'm not sure of Python 2.7. Not only the backport is not straighforward, but I'm not sure that I want to touch the super-stable 2.7 branch. Maybe for 2.7, I should just remove the currently skipped test_bpo20891() test from test_capi.

      What do you think?

    26. vstinner commented on Jan 29, 2018

      @vstinner
      vstinner
      on Jan 29, 2018
      Member

      Antoine Pitrou considers that my PR 5421 for Python 3.6 should not be merged:
      "@vstinner I don't think so. People can already call PyEval_InitThreads."
      #5421 (comment)

    27. vstinner commented on Jan 29, 2018

      @vstinner
      vstinner
      on Jan 29, 2018
      Member

      New changeset d951157 by Victor Stinner in branch 'master':
      bpo-20891: Reenable test_embed.test_bpo20891() (GH-5420)
      d951157

    28. vstinner commented on Jan 29, 2018

      @vstinner
      vstinner
      on Jan 29, 2018
      Member

      Antoine Pitrou:

      @vstinner I don't think so. People can already call PyEval_InitThreads.

      Since only two users complained about https://bugs.python.org/issue20891 in 3 years, I agree that it's ok to not fix Python 2.7 and 3.6. The workaround is to call PyEval_InitThreads() before starting the first thread.

      I wasn't excited to make such stable in stable 2.7 and 3.6 anyway :-)

      test_embed.test_bpo20891() is enabled again on master. I now close the issue.

      Thanks Steve Dower for the bug report!

    29. vstinner
      closed this as completedon Jan 29, 2018

    30. vstinner commented on Jan 29, 2018

      @vstinner
      vstinner
      on Jan 29, 2018
      Member

      New changeset 0cecc22 by Victor Stinner in branch '3.6':
      bpo-20891: Remove test_capi.test_bpo20891() (bpo-5425)
      0cecc22

    31. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    No one assigned

      Labels

      3.7 (EOL)end of lifeextension-modulesC modules in the Modules dir

      Projects

      No projects

        Milestone

        No milestone

        Relationships

        None yet

        Development

        No branches or pull requests

        Issue actions