bpo-31356: Add context manager to temporarily disable GC #4224

pablogsal · 2017-11-02T00:24:51Z

As discussed in the issue by @serhiy-storchaka , @rhettinger , @ncoghlan and @gpshead this PR implements a context manager that temporarily disables GC and restores it to its previous state. The implementation is done in C, using the GIL as the locking mechanism to evict the thread problems discussed in the issue.

The documentation is updated as well as the test suite to take this into account.

Please, indicate any change that is needed and I will be more than happy to change it. 😄

https://bugs.python.org/issue31356

SylvainDe · 2017-11-02T09:21:40Z

Modules/gcmodule.c

+new_thing(PyTypeObject *type, PyObject *args, PyObject *kwdict){
+    disabled_object *self;
+    self = (disabled_object *)type->tp_alloc(type, 0);
+    return (PyObject *) self;


Out of curiosity, is this any different from a single statement:

return (PyObject *) type->tp_alloc(type, 0);

Not in this case (unless I am missing something) because I am not doing anything special with the object itself before returning but is the convention when calling tp_alloc. Check for example:

https://github.com/python/cpython/blob/master/Modules/itertoolsmodule.c#L37

Cool, thanks for the quick answer :)

vadmium · 2017-11-11T20:59:51Z

Doc/library/gc.rst

+         # such as a pair trade, robotic braking, etc
+
+   without needing to explicitly enable and disable the garbage collector yourself.
+   This context manager is implemented in C to assure atomicity, thread safety and speed.


It would be good to clarify what atomicity and thread safety is assured (if any). I understand there is just one GC setting that applies to all threads.

What happens if this context manager is used in two threads at once? In particular, if thread B exiting the with statement can enable GC when thread A is still inside a with statement, that does not seem “safe”. What is the effect of calling gc.disable while another thread runs the context manager? Is GC re-enabled at __exit__, or is it disabled indefinitely?

I will complement the documentation explaining this. Just to clarify the current behaviour: The context manager deactivates the garbage collector and restores the previous state of the garbage collector in the exit method. So if one thread uses the context manager while other has called it already, the garbage collector will be kept deactivated at the exit of the second invocation and it will be reactivated only at the end of the first (assuming that it was activated initially). So the guarantee is: the garbage collector will be deactivated at the entering of the context manager and the initial state will be restored at the end. This means that a thread can never reactivate the garbage collector at exit if it entered with the garbage collector deactivated. Calling gc.disable in a thread if another is inside the context manager has no effect (the gc will be kept disabled and it won't change the behaviour at exit in the thread that started the context manager).

As far as thread-safety goes: the CM is just as thread-unsafe as doing this dance manually. That means if thread A enters first, then thread B enters, then thread A exits, the gc will be turned back on even though thread B is still inside the context manager.

While we could choose to make this more thread-safe by replacing the CM-local "Was the GC enabled?" flag with a process-global counted semaphore:

that still isn't entirely thread-safe due to the possibility of direct calls to the underlying APIs

it adds pointless overhead to the single-threaded case

What could be useful though is a "gc.isenabled()" check in exit that emits RuntimeWarning if the GC is already enabled at that point. That way, while the CM wouldn't magically fix your thread safety problems, it would help you find out you had them in the first place.

+1 to the gc.isenabled() check at exit. Should I proceed to implement it?

Notice that this still have edge cases for example:

Thread A enters (gc disabled)

Thread B enters (gc disabled)

Thread A exits (gc enabled) <-- bad

Thread C enters (gc disabled)

Thread C exists (gc disabled)

Thread B exists (gc disabled)

In this example, the hypothetical RuntimeWarning will never get triggered and thread B will never know that is has been running with the gc disabled for a while.

Right, the documentation should still say the context manager is not thread-safe, and can't be readily made so due to existing code that calls the low-level APIs directly.

https://www.python.org/dev/peps/pep-0556/ is likely to be a better way forward for handling GC pauses in a thread-safe way than ad hoc disabling of the GC around performance critical bits of code.

@ncoghlan In 00222b4 you will find a way to emit a warning if the garbage collector is activated when some thread is inside the gc.ensure_disabled context + tests. Please, review it and if its ok I will update the documentation with the current behaviour and caveats. Of course if you prefer a different implementation I am more than happy to change it 😄

Notice that this implementation is really one step away from the counted semaphore and it covers the public API so maybe it makes sense to completely disallow enabling the gc if another thread is in inside the context. A microbenchmark reveals that is only 5.47% +- 3.2 (mean -+ stdv) slower than the previous one.

…gc is activated

pablogsal · 2017-11-29T00:40:21Z

Hi @ncoghlan ! Just a reminder in case this got outside of your radar!

ncoghlan · 2018-01-30T03:32:30Z

Thanks for taking care of that @rhettinger!

And thanks for the PR @pablogsal :)

…honGH-4224)" This reverts commit 72a0d21. The reverted commit had a few issues hene it was unanimously decided to undo it. See the bpo issue for details.

the-knights-who-say-ni added the CLA signed label Nov 2, 2017

bedevere-bot added the awaiting review label Nov 2, 2017

Add first version of the context manager to temporarily disable GC

2d80d83

pablogsal force-pushed the bpo31356 branch 3 times, most recently from 5e263d4 to 411fc6e Compare November 2, 2017 00:42

Add News entry

4c02071

pablogsal force-pushed the bpo31356 branch from 411fc6e to 4c02071 Compare November 2, 2017 00:43

Add new test and fix test style

da600dd

SylvainDe reviewed Nov 2, 2017

View reviewed changes

Simplify allocator

d7b4266

pablogsal force-pushed the bpo31356 branch from 5cc11e7 to d7b4266 Compare November 2, 2017 10:46

pablogsal force-pushed the bpo31356 branch 3 times, most recently from 277ef08 to ea5a219 Compare November 11, 2017 17:23

Rename contextmanager to ensure_disabled

0978387

pablogsal force-pushed the bpo31356 branch from ea5a219 to 0978387 Compare November 11, 2017 17:24

vadmium reviewed Nov 11, 2017

View reviewed changes

Emit a warning if another thread is inside gc.ensure_enabled and the …

00222b4

…gc is activated

rhettinger merged commit 72a0d21 into python:master Jan 29, 2018

bedevere-bot removed the awaiting review label Jan 29, 2018

pablogsal deleted the bpo31356 branch January 29, 2018 21:05

rhettinger mentioned this pull request May 20, 2023

Add context manager to temporarily disable GC #75537

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-31356: Add context manager to temporarily disable GC #4224

bpo-31356: Add context manager to temporarily disable GC #4224

pablogsal commented Nov 2, 2017 •

edited

Loading

SylvainDe Nov 2, 2017

pablogsal Nov 2, 2017 •

edited

Loading

SylvainDe Nov 2, 2017

vadmium Nov 11, 2017

pablogsal Nov 11, 2017 •

edited

Loading

ncoghlan Nov 12, 2017

pablogsal Nov 12, 2017 •

edited

Loading

pablogsal Nov 12, 2017 •

edited

Loading

ncoghlan Nov 12, 2017

pablogsal Nov 13, 2017 •

edited

Loading

pablogsal Nov 13, 2017

pablogsal commented Nov 29, 2017

ncoghlan commented Jan 30, 2018

bpo-31356: Add context manager to temporarily disable GC #4224

bpo-31356: Add context manager to temporarily disable GC #4224

Conversation

pablogsal commented Nov 2, 2017 • edited Loading

SylvainDe Nov 2, 2017

Choose a reason for hiding this comment

pablogsal Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

SylvainDe Nov 2, 2017

Choose a reason for hiding this comment

vadmium Nov 11, 2017

Choose a reason for hiding this comment

pablogsal Nov 11, 2017 • edited Loading

Choose a reason for hiding this comment

ncoghlan Nov 12, 2017

Choose a reason for hiding this comment

pablogsal Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

pablogsal Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

ncoghlan Nov 12, 2017

Choose a reason for hiding this comment

pablogsal Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

pablogsal Nov 13, 2017

Choose a reason for hiding this comment

pablogsal commented Nov 29, 2017

ncoghlan commented Jan 30, 2018

pablogsal commented Nov 2, 2017 •

edited

Loading

pablogsal Nov 2, 2017 •

edited

Loading

pablogsal Nov 11, 2017 •

edited

Loading

pablogsal Nov 12, 2017 •

edited

Loading

pablogsal Nov 12, 2017 •

edited

Loading

pablogsal Nov 13, 2017 •

edited

Loading