[issue31356] Add context manager to temporarily disable GC

[Yonatan Goldschmidt]

Change by Yonatan Goldschmidt :


--
nosy: +Yonatan Goldschmidt

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Bar Harel]

Change by Bar Harel :


--
nosy: +Dennis Sweeney

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Bar Harel]

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 relying on the 
GIL would work.

It's not very efficient atm, as the overhead of the Lock() and 
resulting python code probably offsets the benefit of disabling the GC, but it 
can be fine for some uses I guess.
 
https://github.com/bharel/disable_gc

I'll release it to pypi sometime soon and we can take a look at the 
not-so-accurate usage metric to see if such a thing is needed in the standard 
library.

--
nosy:  -Dennis Sweeney

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Dennis Sweeney]

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

def gen():
with gc_disabled():
yield 1
yield 2
yield 3

a = gen()
b = gen()
next(a) # disable gc
next(b) 
list(a) # this re-enableds the gc
next(b) # gc is enabled but shouldn't be.

A counter for "number of times disabled" may be a better approach.

--
nosy: +Dennis Sweeney

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Bar Harel]

Change by Bar Harel :


--
nosy: +bar.harel

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Serhiy Storchaka]

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 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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Nick Coghlan]

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-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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[pmpp]

Change by pmpp :


--
nosy: +pmpp

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Gregory P. Smith]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Antoine Pitrou]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Serhiy Storchaka]

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 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Yury Selivanov]

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".  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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Gregory P. Smith]

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 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Gregory P. Smith]

Change by Gregory P. Smith :


--
stage: resolved -> needs patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Yury Selivanov]

Change by Yury Selivanov :


--
resolution:  -> rejected
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Yury Selivanov]

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/29fd9eae432a54c963262e895b46f081f238539a


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[miss-islington]

Change by miss-islington :


--
pull_requests: +5329

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Yury Selivanov]

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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Yury Selivanov]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Yury Selivanov]

Change by Yury Selivanov :


--
pull_requests: +5328

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Yury Selivanov]

Yury Selivanov  added the comment:

Raymond, do you need help with reverts?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Nick Coghlan]

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 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)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[YoSTEALTH]

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 what!!!).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Josh Rosenberg]

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 CapWords for class names isn't ironclad, 
particularly when being a class is more implementation detail than design goal.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[YoSTEALTH]

YoSTEALTH  added the comment:

ps, maybe a better name "DisabledZone" ?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[YoSTEALTH]

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

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Ned Deily]

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 me.  Thanks!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Raymond Hettinger]

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 tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Ned Deily]

Change by Ned Deily :


--
Removed message: https://bugs.python.org/msg311359

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Ned Deily]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Ned Deily]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Ned Deily]

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 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).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Raymond Hettinger]

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 tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Yury Selivanov]

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 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Ned Deily]

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.

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!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

I concur with Yury.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Yury Selivanov]

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, 
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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Serhiy Storchaka]

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-party projects try to support warning-free code, this can 
prevent running tests with 3.7.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Yury Selivanov]

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Gregory P. Smith]

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Yury Selivanov]

Yury Selivanov  added the comment:

IMO this needs to be pulled from 3.7.  Ned?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Yury Selivanov]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Yury Selivanov]

Change by Yury Selivanov :


--
pull_requests: +5288

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Serhiy Storchaka]

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/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:  returned a result with an error set

Thread 0x7fb1b1773700 (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 0x7fb1b824f040 (most recent call first):
  File "/home/serhiy/py/cpython/Lib/test/test_gc.py", line 1061 in 
test_ensure_disabled_thread
  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 
  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)

--
nosy: +ned.deily
priority: normal -> release blocker

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Pablo Galindo Salgado]

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 because without the changes in this PR, this proposed test 
will produce this error:

```python

==
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:  returned a result with an error se

```

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Lisa Roach]

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!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Lisa Roach]

Change by Lisa Roach :


--
pull_requests: +5284
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Raymond Hettinger]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Yury Selivanov]

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)
/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 }

--
nosy: +yselivanov

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Yury Selivanov]

Change by Yury Selivanov :


--
resolution: fixed -> 
stage: resolved -> 
status: closed -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Raymond Hettinger]

Change by Raymond Hettinger :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Raymond Hettinger]

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/72a0d218dcc94a3cc409a9ef32dfcd5a7bbcb43c


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Nick Coghlan]

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 would then be used all the way through the code and 
documentation.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Pablo Galindo Salgado]

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 included in the documentation

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Pablo Galindo Salgado]

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. Please, can you indicate what will be a correct name for the 
context manager so I can update the PR?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Serhiy Storchaka]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Pablo Galindo Salgado]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Pablo Galindo Salgado]

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 tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Pablo Galindo Salgado]

Change by Pablo Galindo Salgado :


--
keywords: +patch
pull_requests: +4192
stage: needs patch -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Raymond Hettinger]

Raymond Hettinger added the comment:

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

--
assignee:  -> lisroach
nosy: +lisroach

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Nick Coghlan]

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 tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Josh Rosenberg]

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 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.

--
nosy: +josh.r

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Gregory P. Smith]

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 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Nick Coghlan]

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 tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Serhiy Storchaka]

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

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Nick Coghlan]

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 tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31356] Add context manager to temporarily disable GC

[Raymond Hettinger]

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 would be roughly 
equivalent to:

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

--
assignee: ncoghlan
components: Interpreter Core
messages: 301407
nosy: eric.snow, gregory.p.smith, ncoghlan, rhettinger
priority: normal
severity: normal
status: open
title: Add context manager to temporarily disable GC
type: enhancement
versions: Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com