Add context manager to temporarily disable GC

[rhettinger]

BPO	31356
Nosy	@rhettinger, @gpshead, @ncoghlan, @pitrou, @ned-deily, @pmp-p, @ericsnowcurrently, @serhiy-storchaka, @1st1, @MojoVampire, @YoSTEALTH, @bharel, @lisroach, @pablogsal, @sweeneyde, @Jongy
PRs	python/cpython#4224 python/cpython#5456 python/cpython#5462 python/cpython#5495 python/cpython#5496

^{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: ```python assignee = None closed_at = None created_at = labels = ['interpreter-core', '3.8', 'type-feature', 'library'] title = 'Add context manager to temporarily disable GC' updated_at = user = 'https://github.com/rhettinger' ``` bugs.python.org fields: ```python activity = actor = 'Yonatan Goldschmidt' assignee = 'none' closed = False closed_date = None closer = None components = ['Interpreter Core', 'Library (Lib)'] creation = creator = 'rhettinger' dependencies = [] files = [] hgrepos = [] issue_num = 31356 keywords = ['patch'] message_count = 52.0 messages = ['301407', '301633', '301636', '301644', '301645', '301710', '301720', '302903', '305402', '306053', '306054', '306071', '306072', '306076', '311167', '311300', '311305', '311312', '311331', '311337', '311340', '311342', '311343', '311344', '311347', '311348', '311350', '311351', '311352', '311356', '311357', '311360', '311362', '311363', '311366', '311372', '311396', '311397', '311402', '311444', '311491', '311494', '311495', '311508', '311512', '311513', '311514', '311566', '311635', '311646', '385213', '385214'] nosy_count = 16.0 nosy_names = ['rhettinger', 'gregory.p.smith', 'ncoghlan', 'pitrou', 'ned.deily', 'pmpp', 'eric.snow', 'serhiy.storchaka', 'yselivanov', 'josh.r', 'YoSTEALTH', 'bar.harel', 'lisroach', 'pablogsal', 'Dennis Sweeney', 'Yonatan Goldschmidt'] pr_nums = ['4224', '5456', '5462', '5495', '5496'] priority = 'normal' resolution = None stage = 'needs patch' status = 'open' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue31356' versions = ['Python 3.8'] ```

[rhettinger]

Used like this:

    with gc_disabled():
        run_some_timing()

    with gc_disabled():
        # do_something_that_has_real_time_guarantees
        # such as a pair trade, robotic braking, etc

This would be implemented in C for atomicity and speed. It would be roughly equivalent to:

    @contextmanager
    def gc_disabled():
        if gc.isenabled():
            gc.disable()
            yield
            gc.enable()
        else:
            yield

[ncoghlan]

+1 from me for the general idea. As far as where to put it goes, I think the gc module would be the most appropriate home.

[serhiy-storchaka]

Note that threads can break this. Even without calling gc.enable() explicitly, "with gc_disabled()" in different thread can enable GC inside other "with gc_disabled()" block.

[ncoghlan]

Yes, this will be in the same category as the standard stream redirection context managers - multi-threaded applications either won't be able to use it, or else will need to manage access to it somehow.

[gpshead]

I believe Raymond is aware of the thread issue. We discussed it.

If gc.disable() would return the previous state of the gc instead of None and an API to enable based on a passed in bool, both of which are written in C and executed with the GIL (or merely another lock protecting changing the gc state rather than the GIL) held and this Python would works with multiple threads all fighting to control the gc state:

@contextmanager
def gc_disabled():
  previous_enable_state = gc.disable()
  yield
  gc.set_enable(previous_enable_state)

But we don't need to modify gc.disable's return value or add a set_enable() if gc_disabled() itself is not implemented in Python. That's up to the implementer.

[99ffcaa5-b43b-4e8e-a35e-9c890007b9cd]

I'd be -1 on this without a demonstrated broad need for this in at least some context outside of microbenchmarking utilities (which presumably have already implemented similar stuff).

If a minimum bar for applicability isn't applied, we'll end up with dozens of these special purpose managers justified by the last limited applicability one. Stuff like contextlib.closing is justifiable (though I wish it allowed you to replace the method name called, so it could call terminate, kill, release, what-have-you) since cleanup close is so common, but disabling and reenabling gc is usually for timing purposes, and there aren't *that* many tools that need it.

It doesn't seem all that useful for real time purposes either; sure, it disables gc, but it doesn't disable other forms of implicit non-local code execution that are surprisingly hard to predict (e.g. object destruction, including __del__, for non-cyclic cases is going to depend on whether the stuff being decref-ed is still owned outside the block). Disabling GC means a lot in Java, because *all cleanup is GC; in Python, it's just cycle collection, so you're not giving much in the way of guarantees, as the code in question has to be written with a *lot of assumptions that Python usually can't support (it's hardly a realtime language).

Seems like if you want a speed up and reliability for this case (and all other context managers intended to be low overhead), it would be better to move parts of contextlib to the C layer (e.g. contextmanager and the classes it's implemented in terms of), so the simple and correct throwaway implementations for arbitrary custom use cases are fast enough; atomicity clearly doesn't actually matter here (it can't be made atomic in any meaningful sense given the lack of thread safety), so any real problem with the provided implementations (assuming they had try/finally added to make them robust) is overhead, not atomicity; the code posted so far would be fine (after adding try/finally) without the speed issues.

[ncoghlan]

This is being proposed for the gc module, not contextlib, so I'm not sure how the previous comment applies.

This is just a convenience API for folks already doing this kind of manipulation themselves.

[rhettinger]

Assigning this to Lisa for a C implementation, docs, and tests.

[pablogsal]

I have prepared a PR in GitHub with an initial implementation of the context manager trying to fulfil the discussed requirements: https://github.com/python/cpython/pull/3980

[pablogsal]

I just realize that the link I provided is incorrect. This is the correct one (also is the one that appears in this issue anyway):

https://github.com/python/cpython/pull/4224

[serhiy-storchaka]

What is the name of the context manager? gc_disabled, disabled, or Disabled? I see different names in the PR and this confuses me.

[pablogsal]

Sorry about that. The context manager is "gc.Disabled()", which I admit is probably a bad name. The documentation is an example of the "equivalent" Python code as stated by Raymond in the first comment but I see now that this will raise confusion. Please, can you indicate what will be a correct name for the context manager so I can update the PR?

[pablogsal]

Just to clarify the current situation: At this point, the contextmanager is referred as "disabled" in the C code but is exported as "Disabled" to the garbage collection module. The "gc_disabled" is the Python "equivalent" given by Raymond that is included in the documentation

[ncoghlan]

Given the existing "gc.enable", "gc.disable" and "gc.isenabled" APIs, I'd suggest calling this one "gc.ensure_disabled": it ensures the GC is disabled in the with statement body, but only turns it back on at the end if it was previously enabled.

That same name would then be used all the way through the code and documentation.

[rhettinger]

New changeset 72a0d218dcc94a3cc409a9ef32dfcd5a7bbcb43c by Raymond Hettinger (Pablo Galindo) in branch 'master': bpo-31356: Add context manager to temporarily disable GC (GH-4224) https://github.com/python/cpython/commit/72a0d218dcc94a3cc409a9ef32dfcd5a7bbcb43c

[1st1]

A bug found by coverity:

(PyErr_WarnEx might error out; please update the code to handle that)

____ ** CID 1428756: Error handling issues (CHECKED_RETURN) /Modules/gcmodule.c: 1071 in gc_enable_impl() 1065 1066 static PyObject 1067 gc_enable_impl(PyObject *module) 1068 /[clinic end generated code: output=45a427e9dce9155c input=81ac4940ca579707]/ 1069 { 1070 if(_PyRuntime.gc.disabled_threads){ 1071 PyErr_WarnEx(PyExc_RuntimeWarning, "Garbage collector enabled while another " 1072 "thread is inside gc.ensure_enabled",1); 1073 } 1074 _PyRuntime.gc.enabled = 1; 1075 Py_RETURN_NONE; 1076 }

[rhettinger]

(PyErr_WarnEx might error out; please update the code to handle that)

Lisa, would you like to take a crack at fixing this one?

[lisroach]

I gave it a shot- looks like we need to ensure that we catch any errors that could be thrown by the warning itself.

I wasn't entirely sure how to create a test for this, if anyone knows how I'll definitely add it!

[pablogsal]

@lisroach A possible test for this is repeat test_ensure_disabled_thread with warnings as errors:

warnings.filterwarnings('error')

and then checking for a RuntimeWarning exception instead of the warning. I think this will work because without the changes in this PR, this proposed test will produce this error:

======================================================================
ERROR: test_ensure_disabled_thread (Lib.test.test_gc.GCTogglingTests)
----------------------------------------------------------------------
RuntimeWarning: Garbage collector enabled while another thread is inside gc.ensure_enabled

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/pgalindo3/cpython/Lib/test/support/__init__.py", line 2083, in decorator
    return func(*args)
  File "/home/pgalindo3/cpython/Lib/test/test_gc.py", line 1063, in test_ensure_disabled_thread
    inside_status_after_thread = gc.isenabled()
SystemError: <built-in method __exit__ of gc.ensure_disabled object at 0x7f0253d22de0> returned a result with an error se

Another posible test is checking that SystemError is not raised / in stderr.

[serhiy-storchaka]

Actually raising an exception in PyErr_WarnEx() is not an error, but a standard behavior with corresponding configuration.

For now running tests with -We in debug build crashes.

$ ./python -We -m test test_gc
Run tests sequentially
0:00:00 load avg: 0.33 [1/1] test_gc
Fatal Python error: a function returned a result with an error set
RuntimeWarning: Garbage collector enabled while another thread is inside gc.ensure_enabled

The above exception was the direct cause of the following exception:

SystemError: \<built-in method __exit__ of gc.ensure_disabled object at 0x7fb1b62a08f8> returned a result with an error set

Thread 0x00007fb1b1773700 (most recent call first): File "/home/serhiy/py/cpython/Lib/test/test_gc.py", line 1050 in disabling_thread File "/home/serhiy/py/cpython/Lib/threading.py", line 865 in run File "/home/serhiy/py/cpython/Lib/threading.py", line 917 in _bootstrap_inner File "/home/serhiy/py/cpython/Lib/threading.py", line 885 in _bootstrap

Current thread 0x00007fb1b824f040 (most recent call first): File "/home/serhiy/py/cpython/Lib/test/test_gc.py", line 1061 in test_ensure_disabledthread File "/home/serhiy/py/cpython/Lib/test/support/init.py", line 2083 in decorator File "/home/serhiy/py/cpython/Lib/unittest/case.py", line 615 in run File "/home/serhiy/py/cpython/Lib/unittest/case.py", line 663 in \_call File "/home/serhiy/py/cpython/Lib/unittest/suite.py", line 122 in run File "/home/serhiy/py/cpython/Lib/unittest/suite.py", line 84 in __call File "/home/serhiy/py/cpython/Lib/unittest/suite.py", line 122 in run File "/home/serhiy/py/cpython/Lib/unittest/suite.py", line 84 in __call File "/home/serhiy/py/cpython/Lib/test/support/init.py", line 1760 in run File "/home/serhiy/py/cpython/Lib/test/support/init.py", line 1861 in _run_suite File "/home/serhiy/py/cpython/Lib/test/support/init.py", line 1951 in run_unittest File "/home/serhiy/py/cpython/Lib/test/test_gc.py", line 1088 in test_main File "/home/serhiy/py/cpython/Lib/test/libregrtest/runtest.py", line 176 in runtest_inner File "/home/serhiy/py/cpython/Lib/test/libregrtest/runtest.py", line 140 in runtest File "/home/serhiy/py/cpython/Lib/test/libregrtest/main.py", line 379 in run_tests_sequential File "/home/serhiy/py/cpython/Lib/test/libregrtest/main.py", line 458 in run_tests File "/home/serhiy/py/cpython/Lib/test/libregrtest/main.py", line 536 in _main File "/home/serhiy/py/cpython/Lib/test/libregrtest/main.py", line 510 in main File "/home/serhiy/py/cpython/Lib/test/libregrtest/main.py", line 585 in main File "/home/serhiy/py/cpython/Lib/test/main__.py", line 2 in \<module> File "/home/serhiy/py/cpython/Lib/runpy.py", line 85 in _run_code File "/home/serhiy/py/cpython/Lib/runpy.py", line 193 in _run_module_as_main Aborted (core dumped)

[1st1]

Just noting here: the original PR was a little bit under-reviewed: return values of C functions were not checked, and the code style was very far from PEP-7.

[1st1]

IMO this needs to be pulled from 3.7. Ned?

[gpshead]

I would not remove this. It is a new feature, leave it in for 3.7.

The style issues and missing error checks are easy to address. We're at 3.7beta1 stage, it is normal to have issues to be cleaned up in a beta release.

[1st1]

The style issues and missing error checks are easy to address. We're at 3.7beta1 stage, it is normal to have issues to be cleaned up in a beta release.

I don't understand the code. Please take a look in my PR.

[serhiy-storchaka]

I don't understand why PyGILState_Ensure() and PyGILState_Release() are used at all. I think it is better to revert PR 4224 until we understood this code.

Currently the code contains a bug which leads to a crash in some circumstances. Since several third-party projects try to support warning-free code, this can prevent running tests with 3.7.

[1st1]

A few thoughts:

1. The merged PR releases GIL for any Python code run in with gc.ensure_disabled(). This is just plain wrong.

2. The merged PR crashes on debug build of CPython.

3. I don't actually understand this feature. Our GC is not per OS thread, which means that inside with gc.ensure_disabled() the GC can become suddenly enabled, if another thread enables it. This API is just misleading (maybe the name of the new context manager is bad—it cannot ensure anything). There's even a unittest that tests this!

4. This new API can be trivially implemented in pure Python:

   @contextmanager
   def disable_gc():
       gc.disable()
       try:
           yield
       finally:
           gc.enable()

5. While such pure Python version shares the problem discussed in (3), the currently committed C implementation doesn't fix them either.

IMO this entire issue needs to be properly debated on python-ideas first and the PR should be reverted from beta-1. If the latter is not possible, OK, let's revert it for beta-2 and for 3.8.

[serhiy-storchaka]

I concur with Yury.

[ned-deily]

2. The merged PR crashes on debug build of CPython.

Actually, for me, it only crashes on a debug build when using -We with tests, an option I don't normally use, otherwise, I would have noticed it before. And, as noted, few downstream users use debug builds.

But, more importantly, this is a new feature so it doesn't break any existing code. If that is the case, I think we don't need to expedite a release; it can wait four weeks for 3.7.0b2 and in that time we need to decide what the proper actions for 3.7 (fix, reimplement, remove) and for 3.8.

Please chime in now if you strongly disagree. In any case, thanks all for your efforts on this!

[1st1]

But, more importantly, this is a new feature so it doesn't break any existing code.

I imagine you're almost done with the beta-1 release, Ned, so I'd hate to create more work for you here. Let's release beta-1 as is.

and in that time we need to decide what the proper actions for 3.7 (fix, reimplement, remove) and for 3.8.

I have a different opinion on this though. The committed PR is **fundamentally** broken (releases GIL for Python code, refleaks, SIGABRT, nonsensical test) and the feature itself is very questionable. As soon as 3.7 branch appears, I'll revert the commit from 3.7 and from master.

If we want this feature to be kept in 3.7beta2, someone should write a **new** patch from scratch. But I strongly suggest to first discuss it on python-dev, because as is, not only the commit is broken, the proposed API is broken too from my standpoint.

[rhettinger]

I also think this should be reverted if that is still possible. I didn't put in sufficient review effort at the outset. No need to publish embarrassing code, especially for such a minor feature.

[ned-deily]

OK, it sounds like we have enough of a consensus to remove it for 3.7.0b2. Unfortunately, 3.7.0b1 is already finished. We are not going to do an emergency brown bag b2 just for this. We could add a warning note to the release notice that is about to got out but OTOH the feature isn't very visible and the documentation for it will disappear from the docs web site once the revert is made (and please wait for the imminent announcement of the 3.7 branch to do that).

[ned-deily]

(If anyone downstream is having problems with test aborts, a workaround until b2 is to simply skip test_gc by adding -x test_gc, like:

python3.7 -m test -We -x test_gc

)

[rhettinger]

Agree that this is invisible enough that no release note is warranted.

Can I go ahead and do a reversion on the master branch or I should I wait a day or so?

[ned-deily]

Agree that this is invisible enough that no release note is warranted.

OK

Can I go ahead and do a reversion on the master branch or I should I wait a day or so?

Please hold off for the moment until the 3.7 branch is announced. It will make life easier for me. Thanks!

[b9e3e125-c758-471e-8446-bb5afa99e9cd]

Since "ensure_disabled" is a class https://docs.python.org/3.7/library/gc.html#gc.ensure_disabled it should be "EnsureDisabled" according to https://www.python.org/dev/peps/pep-0008/#class-names

[b9e3e125-c758-471e-8446-bb5afa99e9cd]

ps, maybe a better name "DisabledZone" ?

[99ffcaa5-b43b-4e8e-a35e-9c890007b9cd]

YoSTEALTH: I think this is one of those cases where the fact of it being a class is incidental; it's used the same as if it were a function, and just happens to be implemented as a class (the docs actually describe it in terms of a function). PEP-8 guidance on CapWords for class names isn't ironclad, particularly when being a class is more implementation detail than design goal.

[b9e3e125-c758-471e-8446-bb5afa99e9cd]

Actually i don't remember the last time where i saw anyone call a function using a "with" statement. This is very sloppy, sure PEP-8 isn't ironclad but this is going to lead to confusion and we might have another case of datetime model (where you don't know whats what!!!).

[ncoghlan]

A useful heuristic is that if something is named with CapWords, then class-like *usage* is explicitly supported (inheritance, isinstance checks, etc).

If it's named with snake_case or wordsruntogether, then calling it is OK, but you may run into quirky behaviour in class-style usage (e.g. it may not be a class at all in some implementations, or it may not be friendly to subclassing even when it is a full class).

So for something like this, snake_case is appropriate - you're meant to just use it, not subclass it. The fact that it's a type instance is an implementation detail.

(In other cases like contextlib.contextmanager we've made that implementation details status completely explicit in the code by having the public API be a wrapper function that returns an instance of a private class)

[1st1]

Raymond, do you need help with reverts?

[1st1]

Since I'm going to be unavailable for the next 10 days, and I don't want this to be accidentally forgotten, I'll do the revert myself. Opened a PR for that.

[1st1]

New changeset 383b32fe108ea627699cc9c644fba5f8bae95d73 by Yury Selivanov in branch 'master': Revert "bpo-31356: Add context manager to temporarily disable GC python/cpython#49745 https://github.com/python/cpython/commit/383b32fe108ea627699cc9c644fba5f8bae95d73

[1st1]

New changeset 29fd9eae432a54c963262e895b46f081f238539a by Yury Selivanov (Miss Islington (bot)) in branch '3.7': Revert "bpo-31356: Add context manager to temporarily disable GC python/cpython#49745 (bpo-5496) https://github.com/python/cpython/commit/29fd9eae432a54c963262e895b46f081f238539a

[gpshead]

The idea which this issue represents is not rejected. It is a good one, we found a need for it during the dev sprint last September.

[1st1]

The idea which this issue represents is not rejected. It is a good one, we found a need for it during the dev sprint last September.

Well, not everybody thinks it is a good one. I, for instance, don't think it's a good idea, so it is at least one "-1". I saw Serhiy was unsure about this new feature too, so maybe there are two "-1"s; I don't know. So I kindly ask(-ed) for it to be openly discussed on python-dev, instead of making a unilateral decision.

The problem with the current design is that GC can become enabled inside the 'with' block at any time:

    with gc.ensure_disabled():
        # gc.is_enabled() might be True !

The GC can become enabled from another OS thread, as we have one GC per process. Adding a locking mechanism might be tricky in terms of implementation, and risky in terms of allowing people to accidentally have a deadlock or something. In short, I don't see any way to make this context manager to work reliably in CPython.

Therefore, I think that the best location for a helper like this would be some unittesting helpers collection, as it can work only under some very specific conditions. The core 'gc' module is currently a collection of low-level primitives. I think we need a very solid motivation to add a core GC function that works unreliably and is suitable only for unittesting (at best).

Maybe I'm completely wrong here, in which case I would love to be proved wrong and not just ignored.

[serhiy-storchaka]

I concur with Yury in every point.

The idea looks good for three core developers, but I just don't understand this. This feature looks useless and misleading to me. It is not atomic and doesn't ensure anything, despite its name. If it will be added in the stdlib, there should be very good explanation of its purpose and limitations in the documentation. And I agree that unittest may be better place than gc if the purpose of it is testing.

[pitrou]

Indeed if unit testing is the main use case, I don't really see the point of adding C code. People can code a simple helper using contextlib.contextmanager in a couple of lines.

[gpshead]

Nick and Raymond: do you remember what our original motivating use cases were for this idea during the sprint?

[ncoghlan]

If I recall the discussion correctly, it was:

1. That this was worth doing precisely because the naive approach is likely to be broken in the presence of multiple threads;
2. It was only worth doing either as a true global disable that accounted for multi-threading (e.g. backed by a counted semaphore or the functional equivalent), or else by making gc enable/disable status have a thread local toggle in addition to the global one (so the context manager can ensure "GC is off *in this thread*, regardless of the global status").

Either of those two options requires changes to the main GC machinery though, as otherwise you basically *can't* write a correct context manager for this use case, since a direct call to gc.enable() in another thread would always be problematic.

[serhiy-storchaka]

1. The used approach was broken in the presence of multiple threads too. It didn't guarantee even that GC will be disabled in the next line.

2. What is a sense of disabling GC in a single thread? Objects in Python are not thread local, they are accessible from all threads, and collecting garbage in one thread affects other threads.

For truly disabling GC globally you need to use a counted semaphore or other synchronization primitives, and this can be implemented at Python level. But what are use cases for this context manager? Isn't naive approach enough?