[issue31356] Add context manager to temporarily disable GC

Change by Yonatan Goldschmidt : -- nosy: +Yonatan Goldschmidt ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe:

[issue31356] Add context manager to temporarily disable GC

Change by Bar Harel : -- nosy: +Dennis Sweeney ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.p

[issue31356] Add context manager to temporarily disable GC

Bar Harel added the comment: I've taken a shot making this in pure Python, and took into account a few more factors such as starvation and reentrancy. As Nick said, one of the only ways it can truly work is if it's completely global, in which case we don't need to use the internal lock and r

[issue31356] Add context manager to temporarily disable GC

Dennis Sweeney added the comment: https://bugs.python.org/issue41545 is a duplicate of this. In that report, there's an example of something that can go wrong with the save-a-boolean-per-context-manager approach even when threads are not used, but when concurrent generators are used, like d

[issue31356] Add context manager to temporarily disable GC

Change by Bar Harel : -- nosy: +bar.harel ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python

[issue31356] Add context manager to temporarily disable GC

Serhiy Storchaka added the comment: 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 f

[issue31356] Add context manager to temporarily disable GC

Nick Coghlan added the comment: 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-threa

[issue31356] Add context manager to temporarily disable GC

Change by pmpp : -- nosy: +pmpp ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailma

[issue31356] Add context manager to temporarily disable GC

Gregory P. Smith added the comment: Nick and Raymond: do you remember what our original motivating use cases were for this idea during the sprint? -- ___ Python tracker ___ __

[issue31356] Add context manager to temporarily disable GC

Antoine Pitrou added the comment: 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. -- ___ Python tracker

[issue31356] Add context manager to temporarily disable GC

Serhiy Storchaka added the comment: 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 s

[issue31356] Add context manager to temporarily disable GC

Yury Selivanov added the comment: > 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".

[issue31356] Add context manager to temporarily disable GC

Gregory P. Smith added the comment: 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. -- priority: release blocker -> normal resolution: rejected -> status: closed -> open versions: +Python 3.8 -Python

[issue31356] Add context manager to temporarily disable GC

Change by Gregory P. Smith : -- stage: resolved -> needs patch ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe:

[issue31356] Add context manager to temporarily disable GC

Change by Yury Selivanov : -- resolution: -> rejected stage: patch review -> resolved status: open -> closed ___ Python tracker ___

[issue31356] Add context manager to temporarily disable GC

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

[issue31356] Add context manager to temporarily disable GC

Change by miss-islington : -- pull_requests: +5329 ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://mai

[issue31356] Add context manager to temporarily disable GC

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

[issue31356] Add context manager to temporarily disable GC

Yury Selivanov added the comment: 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. -- assignee: rhettinger -> ___ Python tracker

[issue31356] Add context manager to temporarily disable GC

Change by Yury Selivanov : -- pull_requests: +5328 ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://mai

[issue31356] Add context manager to temporarily disable GC

Yury Selivanov added the comment: Raymond, do you need help with reverts? -- ___ Python tracker ___ ___ Python-bugs-list mailing lis

[issue31356] Add context manager to temporarily disable GC

Nick Coghlan added the comment: 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 behaviou

[issue31356] Add context manager to temporarily disable GC

YoSTEALTH added the comment: Actually i don't remember the last time where i saw anyone call a function using a "with" statement. This is very sloppy, sure PEP8 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 w

[issue31356] Add context manager to temporarily disable GC

Josh Rosenberg added the comment: 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). PEP8 guidance on

[issue31356] Add context manager to temporarily disable GC

YoSTEALTH added the comment: ps, maybe a better name "DisabledZone" ? -- ___ Python tracker ___ ___ Python-bugs-list mailing list Un

[issue31356] Add context manager to temporarily disable GC

YoSTEALTH added the comment: 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 -- nosy: +YoSTEALTH ___

[issue31356] Add context manager to temporarily disable GC

Ned Deily added the comment: > 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

[issue31356] Add context manager to temporarily disable GC

Raymond Hettinger added the comment: 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? -- assignee: lisroach -> rhettinger ___ Python tracke

[issue31356] Add context manager to temporarily disable GC

Change by Ned Deily : -- Removed message: https://bugs.python.org/msg311359 ___ Python tracker ___ ___ Python-bugs-list mailing list

[issue31356] Add context manager to temporarily disable GC

Ned Deily added the comment: (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 ) -- ___ Python tracker

[issue31356] Add context manager to temporarily disable GC

Ned Deily added the comment: (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". ) -- ___ Python tracker

[issue31356] Add context manager to temporarily disable GC

Ned Deily added the comment: 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

[issue31356] Add context manager to temporarily disable GC

Raymond Hettinger added the comment: 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. -- ___ Python

[issue31356] Add context manager to temporarily disable GC

Yury Selivanov added the comment: > 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

[issue31356] Add context manager to temporarily disable GC

Ned Deily added the comment: > 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. B

[issue31356] Add context manager to temporarily disable GC

Serhiy Storchaka added the comment: I concur with Yury. -- ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: ht

[issue31356] Add context manager to temporarily disable GC

Yury Selivanov added the comment: 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, w

[issue31356] Add context manager to temporarily disable GC

Serhiy Storchaka added the comment: 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-pa

[issue31356] Add context manager to temporarily disable GC

Yury Selivanov added the comment: > 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. --

[issue31356] Add context manager to temporarily disable GC

Gregory P. Smith added the comment: 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. --

[issue31356] Add context manager to temporarily disable GC

Yury Selivanov added the comment: IMO this needs to be pulled from 3.7. Ned? -- ___ Python tracker ___ ___ Python-bugs-list mailing

[issue31356] Add context manager to temporarily disable GC

Yury Selivanov added the comment: 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. -- ___ Python tracker

[issue31356] Add context manager to temporarily disable GC

Change by Yury Selivanov : -- pull_requests: +5288 ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://mai

[issue31356] Add context manager to temporarily disable GC

Serhiy Storchaka added the comment: 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

[issue31356] Add context manager to temporarily disable GC

Pablo Galindo Salgado added the comment: @lisroach A possible test for this is repeat `test_ensure_disabled_thread` with warnings as errors: ```python warnings.filterwarnings('error') ``` and then checking for a `RuntimeWarning` exception instead of the warning. I think this will work becaus

[issue31356] Add context manager to temporarily disable GC

Lisa Roach added the comment: 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! --

[issue31356] Add context manager to temporarily disable GC

Change by Lisa Roach : -- pull_requests: +5284 stage: -> patch review ___ Python tracker ___ ___ Python-bugs-list mailing list Unsub

[issue31356] Add context manager to temporarily disable GC

Raymond Hettinger added the comment: > (PyErr_WarnEx might error out; please update the code to handle that) Lisa, would you like to take a crack at fixing this one? -- ___ Python tracker ___

[issue31356] Add context manager to temporarily disable GC

Yury Selivanov added the comment: A bug found by coverity: (PyErr_WarnEx might error out; please update the code to handle that) *** CID 1428756: Error handling issues (CHECKED_RETURN) /M

[issue31356] Add context manager to temporarily disable GC

Change by Yury Selivanov : -- resolution: fixed -> stage: resolved -> status: closed -> open ___ Python tracker ___ ___ Python-bugs

[issue31356] Add context manager to temporarily disable GC

Change by Raymond Hettinger : -- resolution: -> fixed stage: patch review -> resolved status: open -> closed ___ Python tracker ___

[issue31356] Add context manager to temporarily disable GC

Raymond Hettinger added the comment: 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/72a0d218dcc94a3cc409a9ef32dfcd5a7bbcb

[issue31356] Add context manager to temporarily disable GC

Nick Coghlan added the comment: 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

[issue31356] Add context manager to temporarily disable GC

Pablo Galindo Salgado added the comment: 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 inclu

[issue31356] Add context manager to temporarily disable GC

Pablo Galindo Salgado added the comment: 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. Plea

[issue31356] Add context manager to temporarily disable GC

Serhiy Storchaka added the comment: What is the name of the context manager? gc_disabled, disabled, or Disabled? I see different names in the PR and this confuses me. -- ___ Python tracker __

[issue31356] Add context manager to temporarily disable GC

Pablo Galindo Salgado added the comment: 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 -- ___ Python tracker

[issue31356] Add context manager to temporarily disable GC

Pablo Galindo Salgado added the comment: 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 -- nosy: +pablogsal ___ Python tr

[issue31356] Add context manager to temporarily disable GC

Change by Pablo Galindo Salgado : -- keywords: +patch pull_requests: +4192 stage: needs patch -> patch review ___ Python tracker ___

[issue31356] Add context manager to temporarily disable GC

Raymond Hettinger added the comment: Assigning this to Lisa for a C implementation, docs, and tests. -- assignee: -> lisroach nosy: +lisroach ___ Python tracker ___ ___

[issue31356] Add context manager to temporarily disable GC

Nick Coghlan added the comment: 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. -- ___ Python t

[issue31356] Add context manager to temporarily disable GC

Josh Rosenberg added the comment: 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 o

[issue31356] Add context manager to temporarily disable GC

Gregory P. Smith added the comment: 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 an

[issue31356] Add context manager to temporarily disable GC

Nick Coghlan added the comment: 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. -- ___ Python

[issue31356] Add context manager to temporarily disable GC

Serhiy Storchaka added the comment: 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. -- nosy: +pitrou, serhiy.storchaka ___

[issue31356] Add context manager to temporarily disable GC

Nick Coghlan added the comment: +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. -- assignee: ncoghlan -> components: +Library (Lib) stage: -> needs patch ___ Python tracke

[issue31356] Add context manager to temporarily disable GC

New submission from Raymond Hettinger: 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