GeezerVison

Documentation build overview

📚 cpython-previews | 🛠️ Build #33006727 | 📁 Comparing afd0746 against main (2f064fb)

  🔍 Preview build  

Something is wrong with the change. Example on the Windows CI:

test_global_lookup_error (test.test_pickle.CPicklingErrorTests.test_global_lookup_error) ...
Windows fatal exception: access violation

Current thread 0x00000f90 (most recent call first):
  File "D:\a\cpython\cpython\Lib\test\test_pickle.py", line 66 in dumps
  File "D:\a\cpython\cpython\Lib\test\pickletester.py", line 1850 in test_global_lookup_error

Indeed, I got a bit too trigger-happy on the Py_DECREF and so sometimes modules was being dec-ref'd twice. Should be good now, I'll keep an eye on the CI.

Try to avoid force push, it makes reviews harder to follow in GitHub.

Noted, I'll keep that in mind for the future.

CI is green now, thanks for the heads up! :)

There was no critical section in that code, so the original issue is not applicable here. This code is just not free-threading ready. Adding a critical section would be a new feature.

Adding Py_INCREFs after PyDict_CheckExact() is a different story. Yes, there is a possible use-after-free bug reproducible without free-threading.

I see -- if I understand correctly there should never have been a 3.13 backport because the function wasn't supposed to support free-threading in 3.13, right?

Do you want me to change the PR to remove the current changes and make the function hold strong references or should this go to a different PR altogether (and this one be closed)? (I feel like the usual practice on CPython would be the latter, but asking anyway.)

Yes, a different PR would be better.