Use PyEval_InitThreads() as intended

[rwgk]

Description

This PR actually matters

• only for Python 3.6 (according to the Python documentation),
• only with -DPYBIND11_SIMPLE_GIL_MANAGEMENT=ON (evidently, based on CI flakes observed since PR Add PYBIND11_SIMPLE_GIL_MANAGEMENT option (cmake, C++ define) #4216 was merged),
• and only if embed.h is used.

Hypothesis: -DPYBIND11_SIMPLE_GIL_MANAGEMENT=OFF covers up that PyEval_InitThreads() was misplaced.

This PR almost does not matter anymore at all:

• https://docs.python.org/3.11/c-api/init.html#c.PyEval_InitThreads

Changed in version 3.7: This function is now called by Py_Initialize(), so you don’t have to call it yourself anymore.

Note that Python 3.6 EOL was 2021-12-23 (https://endoflife.date/python), but since PR #4216 was merged we had distracting CI flakes that this PR aims to get rid of, e.g.:

• 🐍 3.6 • windows-2022 • x64

Run cmake --build .  --target cpptest -j 2
MSBuild version 17.3.1+2badb37d1 for .NET Framework
  Checking File Globs
  external_module.vcxproj -> D:\a\pybind11\pybind11\tests\test_embed\external_module.cp36-win_amd64.pyd
  test_embed.vcxproj -> D:\a\pybind11\pybind11\tests\test_embed\Debug\test_embed.exe
CUSTOMBUILD : Fatal Python error : This thread state must be current when releasing [D:\a\pybind11\pybind11\tests\test_embed\cpptest.vcxproj]
  
  Current thread 0x0000[14](https://github.com/pybind/pybind11/actions/runs/3511087576/jobs/5881522679#step:13:15)84 (most recent call first):
  
  Thread 0x00000fc4 (most recent call first):
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Microsoft\VC\v170\Microsoft.CppCommon.targets(247,5): error MSB8066: Custom build for 'D:\a\pybind11\pybind11\CMakeFiles\bf7d1[16](https://github.com/pybind/pybind11/actions/runs/3511087576/jobs/5881522679#step:13:17)f1[18](https://github.com/pybind/pybind11/actions/runs/3511087576/jobs/5881522679#step:13:19)90487c30aedc390ae4de7\cpptest.rule;D:\a\pybind11\pybind11\tests\test_embed\CMakeLists.txt' exited with code -1073740791. [D:\a\pybind11\pybind11\tests\test_embed\cpptest.vcxproj]
Error: Process completed with exit code 1.

Suggested changelog entry:

Bug fix affecting only Python 3.6 under very specific, uncommon conditions: move ``PyEval_InitThreads()`` call to the correct location.

[rwgk]

@eacousineau there is no specific reason for requesting a review from you, but @henryiii has been unresponsive for 3 weeks. It would be great if you could help out with a second set of eyes.

[EricCousineau-TRI]

Sounds good, and thanks for the writeup! Though I'm afraid I may be a bit slow to catch up on the issues.

It's hard for me to posit from above Windows-based failure that the above indicated misplacement. Can I ask if the following is the correct reasoning, reasoning within Python 3.6?

• Calling PyEval_InitThreads() in get_internals() via nominal usage did nothing per 3.6 docs stating that "This is a no-op when called for a second time", so this is a harmless (but noisy) misplacement.
• The missing call in embed.h are why Windows (and others?) fail. This smells like it'd happen when the embedded interpreter exits before get_internals() was called. Is this the case? (is it spurious, or deterministic?)

[EricCousineau-TRI]

Still not sure on the whole of the changes (per questions in first post), but def. won't block progress - thanks!

[rwgk]

Still not sure on the whole of the changes (per questions in first post), but def. won't block progress - thanks!

Sorry I wasn't able to finish the below before a long meeting, continuing now ...

Sounds good, and thanks for the writeup! Though I'm afraid I may be a bit slow to catch up on the issues.

It's hard for me to posit from above Windows-based failure that the above indicated misplacement. Can I ask if the following is the correct reasoning, reasoning within Python 3.6?

• Calling PyEval_InitThreads() in get_internals() via nominal usage did nothing per 3.6 docs stating that "This is a no-op when called for a second time", so this is a harmless (but noisy) misplacement.

Yes, in the sense that it is harmless if there is actually a correct placement. That's not the case before this PR. I was thinking of "misplaced" as in "we're moving it from the wrong place to the right place".

• The missing call in embed.h are why Windows (and others?) fail. This smells like it'd happen when the embedded interpreter exits before get_internals() was called. Is this the case? (is it spurious, or deterministic?)

Not deterministic. "flaky" is the term I see getting used around Google.

My understanding is not complete, but IIUC the Python documentation correctly, prior to Python 3.7, PyEval_InitThreads() needs to be called before the first attempt to acquire the GIL. get_internals() clearly violates that, because this:

• pybind11/include/pybind11/detail/internals.h

  Line 433 in 296615a

  gil_scoped_acquire gil;

is before this:

• pybind11/include/pybind11/detail/internals.h

  Line 472 in 296615a

  PyEval_InitThreads();

Now pure speculation: I think that the "non-simple" code in gil.h covered that up, somehow. I'm thinking that because we didn't see the flakiness I'm trying to fix with this PR before PR #4216.

Thanks for the approvals!

[EricCousineau-TRI]

Sweet, that all makes sense - thanks for the thorough response!