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

Scratch PR for reducing PyPy issue observed under #4246 #4268

Closed

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Oct 21, 2022

Description

WIP

Suggested changelog entry:

@rwgk rwgk force-pushed the assert_gil_held_incref_decref_pypy_windows branch from 4817e4c to 910e3ae Compare October 21, 2022 05:16
@rwgk
Copy link
Collaborator Author

rwgk commented Oct 21, 2022

Hi @mattip, after a round of brute-force-trial-and-error I'm thinking there are two problems.

  1. the failed asserts you found (pybind11::handle inc_ref() & dec_ref() PyGILState_Check() **excluding** nullptr #4246 (comment))
  2. pytest hangs in or after its tear down

We can xfail the 1.

But we cannot xfail the 2.
So I took a guess:

If you look in this PR, test_gil_scoped.py has only this left:

from pybind11_tests import gil_scoped as m


def test_cross_module_gil():
    """Makes sure that the GIL can be acquired by another module from a GIL-released state."""
    m.test_cross_module_gil()  # Should not raise a SIGSEGV

I also added some fprintf() to test_gil_scoped.cpp: that code block actually runs to completion (see log below).

But something in the pytest process gets upset somehow and the process hangs, I don't know for how long, I'm guessing "forever". I'll let it timeout this time; before I always cancelled manually.

Taking another guess:

I suspect there is something between

  • here and
  • here that upsets PyPy, and only on Windows, and only with my extra PyGILState_Check()s in pytypes.h

Do those two pretty small pieces of code ring any bells?


Copy of section in CI log:

============================= test session starts =============================
  platform win32 -- Python 3.9.10[pypy-7.3.9-final], pytest-7.0.0, pluggy-1.0.0
  C++ Info: MSVC 193331630 C++14 __pybind11_internals_v4_msvc_debug__
  rootdir: D:\a\pybind11\pybind11\tests, configfile: pytest.ini
  plugins: timeout-2.1.0
  timeout: 300.0s
  timeout method: thread
  timeout func_only: False
  collected 23 items
  
  test_exceptions.py .......x.........s....                                [ 95%]
  test_gil_scoped.py 
  LOOOK D:\a\pybind11\pybind11\tests\test_gil_scoped.cpp:41
  
  LOOOK D:\a\pybind11\pybind11\tests\test_gil_scoped.cpp:44
  
  LOOOK D:\a\pybind11\pybind11\tests\test_gil_scoped.cpp:48
  
  LOOOK D:\a\pybind11\pybind11\tests\test_gil_scoped.cpp:51
  
  LOOOK D:\a\pybind11\pybind11\tests\test_gil_scoped.cpp:54
  .                                                     [100%]
  
  =========================== short test summary info ===========================
  SKIPPED [1] test_exceptions.py:306: PyErr_NormalizeException Segmentation fault
  XFAIL test_exceptions.py::test_python_alreadyset_in_destructor
    Failure on PyPy 3.8 (7.3.7)
  ================== [21](https://github.com/pybind/pybind11/actions/runs/3295069314/jobs/5433235750#step:12:22) passed, 1 skipped, 1 xfailed in 1.80s ===================

******** JOB HANGS HERE FOR A LONG TIME ********

@rwgk
Copy link
Collaborator Author

rwgk commented Oct 21, 2022

The job was still hanging after 2h 4m. I cancelled it, getting worried about burning our CI quota for no good reason.

@mattip
Copy link
Contributor

mattip commented Oct 21, 2022

Nothing there looks familiar. To move ahead, perhaps skip the tests on windows + pypy

@rwgk
Copy link
Collaborator Author

rwgk commented Oct 21, 2022

Nothing there looks familiar. To move ahead, perhaps skip the tests on windows + pypy

Thanks for looking again. PR #4246 already has an #ifdef to disable the new feature for windows + pypy, i.e. this issue is not blocking. I asked JIC a fix is easily within reach, but it's pretty clear now that's not the case. I'll add a reference to this PR to the #ifdef, in case someone wants to work on it later.

@rwgk rwgk closed this Oct 21, 2022
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Oct 21, 2022
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Nov 1, 2022
rwgk pushed a commit that referenced this pull request Dec 9, 2022
…xcluding** `nullptr` (#4246)

* pybind11/pytypes.h `inc_ref()`, `dec_ref()` `PyGILState_Check()` **excluding** `nullptr`

Guarded by `PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF`

* Disable `PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF` for PyPy under Windows.

* Add reference to PR #4268 (PyPy Windows)
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.

2 participants