C API: `PyGC_Disable()` not respected #116604

wjakob · 2024-03-11T15:01:19Z

Bug report

Bug description:

The Python C API provides the function PyGC_Disable() to temporarily disable garbage collection. Calling it causes PyGC_Collect() to become a no-op. So far so good.

Unfortunately, CPython >= 3.12 no longer respects this flag beyond direct calls to PyGC_Collect(). Let's consider, for example, the operation PyErr_CheckSignals() at line 1773. This calls _Py_RunGC, which ignores the GC enabled flag. And this indeed happens! In fact, I just spent quite a bit of time debugging an issue where the GC was supposed to be disabled for a brief moment, and yet it runs on Python 3.12. (Whether disabling the GC for a brief period of time is a good design pattern is another discussion. I would like to steer the discussion away from this and focus on documented API behavior.)

The PyErr_CheckSignals() function is called from 135 locations in the CPython codebase including very common ones like PyObject_Str(), so making any fixes in callers of this API does not look feasible.

The flag-ignoring _Py_RunGC() function is only called by two places: besides the mentioned PyErr_CheckSignals(), there is also _Py_HandlePending() in Python/ceval_gil.c.

To restore the documented behavior, I see three options:

_Py_RunGC() could be modified to exit immediately if the enabled flag is set to zero.
The implementation of _Py_HandlePending() and PyErr_CheckSignals() could be modified to check PyGC_IsEnabled() before calling _Py_RunGC().
Something is setting _PY_GC_SCHEDULED_BIT, and that causes the implementation to enter _Py_RunGC(). I'm not really sure about how that works, but perhaps this flag could be cleared in PyGC_Disable(), and Python could then avoid setting the flag.

(Tagging @pablogsal and @swtaarrs, who commited changes to the relevant code.)

CPython versions tested on:

3.12

Operating systems tested on:

Linux

Linked PRs

The text was updated successfully, but these errors were encountered:

pablogsal · 2024-03-11T16:23:06Z

@wjakob can you confirm if this affects 3.12?

wjakob · 2024-03-11T16:24:12Z

@pablogsal Confirmed. I am on 3.12.2.

PS: I realized that my original bug description had a > 3.12 instead of >= 3.12. Fixed now.

pablogsal · 2024-03-11T17:45:16Z

I will investigate this shortly but if it affects 3.12 I understand exactly what is happening 👍

Signed-off-by: Pablo Galindo <[email protected]>

)

…y_RunGC (pythonGH-116628) (cherry picked from commit 02918aa) Co-authored-by: Pablo Galindo Salgado <[email protected]>

The free-threaded GC only does full collections, so it uses a threshold that is a maximum of a fixed value (default 2000) and proportional to the number of live objects. If there were many live objects after the previous collection, then the threshold may be larger than 10,000 causing `test_indirect_calls_with_gc_disabled` to fail. This manually sets the threshold to `(1000, 0, 0)` for the test. The `0` disables the proprtional scaling.

The free-threaded GC only does full collections, so it uses a threshold that is a maximum of a fixed value (default 2000) and proportional to the number of live objects. If there were many live objects after the previous collection, then the threshold may be larger than 10,000 causing `test_indirect_calls_with_gc_disabled` to fail. This manually sets the threshold to `(1000, 0, 0)` for the test. The `0` disables the proportional scaling.

…readed build This isn't strictly necessary because the implementation of `gc_should_collect` already checks `gcstate->enabled` in the free-threaded build, but it seems like a good idea until the common pieces of gc.c and gc_free_threading.c are refactored out.

… build (#116663) This isn't strictly necessary because the implementation of `gc_should_collect` already checks `gcstate->enabled` in the free-threaded build, but it seems like a good idea until the common pieces of gc.c and gc_free_threading.c are refactored out.

…y_RunGC (pythonGH-116628) (python#116653)

The free-threaded GC only does full collections, so it uses a threshold that is a maximum of a fixed value (default 2000) and proportional to the number of live objects. If there were many live objects after the previous collection, then the threshold may be larger than 10,000 causing `test_indirect_calls_with_gc_disabled` to fail. This manually sets the threshold to `(1000, 0, 0)` for the test. The `0` disables the proportional scaling.

…python#116628)

…readed build (python#116663) This isn't strictly necessary because the implementation of `gc_should_collect` already checks `gcstate->enabled` in the free-threaded build, but it seems like a good idea until the common pieces of gc.c and gc_free_threading.c are refactored out.

The free-threaded GC only does full collections, so it uses a threshold that is a maximum of a fixed value (default 2000) and proportional to the number of live objects. If there were many live objects after the previous collection, then the threshold may be larger than 10,000 causing `test_indirect_calls_with_gc_disabled` to fail. This manually sets the threshold to `(1000, 0, 0)` for the test. The `0` disables the proportional scaling.

…python#116628)

…readed build (python#116663) This isn't strictly necessary because the implementation of `gc_should_collect` already checks `gcstate->enabled` in the free-threaded build, but it seems like a good idea until the common pieces of gc.c and gc_free_threading.c are refactored out.

The free-threaded GC only does full collections, so it uses a threshold that is a maximum of a fixed value (default 2000) and proportional to the number of live objects. If there were many live objects after the previous collection, then the threshold may be larger than 10,000 causing `test_indirect_calls_with_gc_disabled` to fail. This manually sets the threshold to `(1000, 0, 0)` for the test. The `0` disables the proportional scaling.

wjakob · 2024-06-11T07:40:11Z

@pablogsal Should this issue be closed?

pablogsal · 2024-06-11T11:43:46Z

Yep

wjakob added the type-bug An unexpected behavior, bug, or error label Mar 11, 2024

bedevere-app bot mentioned this issue Mar 11, 2024

gh-116604: Correctly honor the gc status when calling _Py_RunGC #116628

Merged

pablogsal added a commit to pablogsal/cpython that referenced this issue Mar 11, 2024

pythongh-116604: Correctly honor the gc status when calling _Py_RunGC

1ff8bec

pablogsal added a commit to pablogsal/cpython that referenced this issue Mar 11, 2024

pythongh-116604: Correctly honor the gc status when calling _Py_RunGC

a887437

Signed-off-by: Pablo Galindo <[email protected]>

pablogsal added a commit to pablogsal/cpython that referenced this issue Mar 11, 2024

pythongh-116604: Correctly honor the gc status when calling _Py_RunGC

9bc0ffc

Signed-off-by: Pablo Galindo <[email protected]>

pablogsal added a commit to pablogsal/cpython that referenced this issue Mar 11, 2024

pythongh-116604: Correctly honor the gc status when calling _Py_RunGC

7e1973c

Signed-off-by: Pablo Galindo <[email protected]>

pablogsal added a commit that referenced this issue Mar 12, 2024

gh-116604: Correctly honor the gc status when calling _Py_RunGC (#116628

02918aa

)

bedevere-app bot mentioned this issue Mar 12, 2024

[3.12] gh-116604: Correctly honor the gc status when calling _Py_RunGC (GH-116628) #116653

Merged

pablogsal added a commit to pablogsal/cpython that referenced this issue Mar 12, 2024

[3.12] pythongh-116604: Correctly honor the gc status when calling _P…

d39d5e4

…y_RunGC (pythonGH-116628) (cherry picked from commit 02918aa) Co-authored-by: Pablo Galindo Salgado <[email protected]>

bedevere-app bot mentioned this issue Mar 12, 2024

gh-116604: Fix test_gc on free-threaded build #116662

Merged

bedevere-app bot mentioned this issue Mar 12, 2024

gh-116604: Check for gcstate->enabled in _Py_RunGC in free-threaded build #116663

Merged

colesbury added a commit to colesbury/cpython that referenced this issue Mar 12, 2024

Merge branch 'pythongh-116604-gc-free-threaded' into nogil-integration

6d417c8

pablogsal added a commit to pablogsal/cpython that referenced this issue Mar 12, 2024

[3.12] pythongh-116604: Correctly honor the gc status when calling _P…

dcfb21d

…y_RunGC (pythonGH-116628) (python#116653)

adorilson pushed a commit to adorilson/cpython that referenced this issue Mar 25, 2024

pythongh-116604: Correctly honor the gc status when calling _Py_RunGC (…

dae8f5a

…python#116628)

diegorusso pushed a commit to diegorusso/cpython that referenced this issue Apr 17, 2024

pythongh-116604: Correctly honor the gc status when calling _Py_RunGC (…

bb10652

…python#116628)

pablogsal closed this as completed Jun 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C API: `PyGC_Disable()` not respected #116604

C API: `PyGC_Disable()` not respected #116604

wjakob commented Mar 11, 2024 •

edited by bedevere-app bot

Loading

pablogsal commented Mar 11, 2024

wjakob commented Mar 11, 2024 •

edited

Loading

pablogsal commented Mar 11, 2024

wjakob commented Jun 11, 2024

pablogsal commented Jun 11, 2024

C API: PyGC_Disable() not respected #116604

C API: PyGC_Disable() not respected #116604

Comments

wjakob commented Mar 11, 2024 • edited by bedevere-app bot Loading

Bug report

Bug description:

CPython versions tested on:

Operating systems tested on:

Linked PRs

pablogsal commented Mar 11, 2024

wjakob commented Mar 11, 2024 • edited Loading

pablogsal commented Mar 11, 2024

wjakob commented Jun 11, 2024

pablogsal commented Jun 11, 2024

C API: `PyGC_Disable()` not respected #116604

C API: `PyGC_Disable()` not respected #116604

wjakob commented Mar 11, 2024 •

edited by bedevere-app bot

Loading

wjakob commented Mar 11, 2024 •

edited

Loading