[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

STINNER Victor  added the comment:

Alexander Mohr reported a memory leak in his code using botocore (Python client 
for Amazon S3): bpo-33565. His code emited ResourceWarning, but these warnings 
are ignored by default. The link between ignored warnings and a memory leak is 
non obvious.

Serhiy: what do you think of backporting my _warnings change for the ignore 
action?

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

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[Alexander Mohr]

Alexander Mohr  added the comment:

not fixing this means that 3.6 slowly leaks for many people in prod.  It's not 
often possible to fix all the warnings on large dynamic applications, I highly 
suggest finding a way to get this into 3.6.  I bet there are a lot of 
frustrated people out there who don't know why their applications slowly leak.

--
nosy: +thehesiod

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

STINNER Victor  added the comment:

I ran a quick benchmark on Python 3.6: current code (3.6 branch, ref) => PR 
4587 (patch):

vstinner@apu$ python3 -m perf compare_to ref.json patch.json 
Mean +- std dev: [ref] 597 ns +- 10 ns -> [patch] 830 ns +- 15 ns: 1.39x slower 
(+39%)

I don't want to backport an optimization to a stable branch, so I prefer to not 
backport the warnings change on the ignore action to Python 3.6. I rejected my 
PR 4587.

The bug was fixed in master. I don't want to backport the change to 2.7 nor 
3.6, so I close the issue.

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

STINNER Victor  added the comment:

Serhiy: "I meant the original issue. AFAIK ResourceWarningis not raised in 2.7. 
And other warnings less likely have unique messages. Seems the problem is less 
critical in 2.7."

Oh yes, sorry, I forgot your comment and I forgot that Python 2.7 doesn't have 
ResourceWarning. I closed my PR for Python 2.7: 
https://github.com/python/cpython/pull/4588

--

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

I meant the original issue. AFAIK ResourceWarningis not raised in 2.7. And 
other warnings less likely have unique messages. Seems the problem is less 
critical in 2.7.

--

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

STINNER Victor  added the comment:

Serhiy: "As for 2.7, does this problem exist in 2.7?"

Yes, see my PR 4588. If you run the test without the fix, the test fails.

--

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

I'm not sure. Ask the RM for 3.6.

As for 2.7, does this problem exist in 2.7?

--

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

STINNER Victor  added the comment:

Victor: "I consider that PR 4489 is a bugfix, so I proposed backports to Python 
2.7 (PR 4588) and 3.6 (PR 4587)."

Serhiy: Are you ok to backport the change to stable branches?

--

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +4511

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

STINNER Victor  added the comment:

I consider that PR 4489 is a bugfix, so I proposed backports to Python 2.7 (PR 
4588) and 3.6 (PR 4587).

--

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

Thank you Victor!

--
assignee: serhiy.storchaka -> vstinner
versions: +Python 3.7 -Python 3.6

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[Roundup Robot]

Change by Roundup Robot :


--
pull_requests: +4510

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

STINNER Victor  added the comment:

https://github.com/python/cpython/pull/4489#issuecomment-346673704

Serhiy: "Could you please make a benchmark for warnings emitted in a tight loop 
in the C code. It would be nice to know what is the worst case. If it is less 
than 2x slower I will prefer this simpler PR."

I merged my PR since it's not 2x slower. I also prefer the simple PR 4489, 
rather than the complex PR 4502.

I rejected my PR 4502 since it's backward incompatible. I don't think that it's 
worth it, even if it's faster.

--

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

STINNER Victor  added the comment:


New changeset c9758784eb321fb9771e0bc7205b296e4d658045 by Victor Stinner in 
branch 'master':
bpo-27535: Fix memory leak with warnings ignore (#4489)
https://github.com/python/cpython/commit/c9758784eb321fb9771e0bc7205b296e4d658045


--

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

STINNER Victor  added the comment:

> These results look strange to me. I expected the same difference but smaller 
> the base time.

Honestly, I was also surprised.

I checked the whole benchmark twice. I also rebased the PR locally to make sure 
that it's not a side effect of a recent change in master.

Results are reliable and reproductible.

FYI I ran the C benchmark on my laptop "apu" using CPU isolation.

--

I reproduced the benchmark on my other "selma" laptop, without CPU isolation 
(so less reliable).

C benchmark:

haypo@selma$ python3 -m perf compare_to master.json ignore.json 
Mean +- std dev: [master] 932 ns +- 66 ns -> [ignore] 1.01 us +- 0.05 us: 1.09x 
slower (+9%)

==> +78 ns

--

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

These results look strange to me. I expected the same difference but smaller 
the base time.

--

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

Change by STINNER Victor :


Added file: https://bugs.python.org/file47287/bench_ignore_warn_c.py

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

STINNER Victor  added the comment:

New benchmark on the emitting a warning which is ignored. Benchmark the PR 4489.


Warning emitted in Python, warnings.warn():

vstinner@apu$ ./python -m perf compare_to master.json ignore.json 
Mean +- std dev: [master] 705 ns +- 24 ns -> [ignore] 838 ns +- 18 ns: 1.19x 
slower (+19%)

==> +133 ns


Warning emitted in C, PyErr_WarnEx():

vstinner@apu$ python3 -m perf compare_to master2.json ignore2.json 
Mean +- std dev: [master2] 702 ns +- 9 ns -> [ignore2] 723 ns +- 9 ns: 1.03x 
slower (+3%)

==> +21 ns


C benchmark, attached files:

* bench_c_warn.patch
* bench_ignore_warn_c.py

--
Added file: https://bugs.python.org/file47286/bench_c_warn.patch

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

STINNER Victor  added the comment:


New changeset b98f1715a35d2cbd1e9e45e1e7ae51a39e00dc4a by Victor Stinner in 
branch 'master':
bpo-27535: Cleanup create_filter() (#4516)
https://github.com/python/cpython/commit/b98f1715a35d2cbd1e9e45e1e7ae51a39e00dc4a


--

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +4453

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

STINNER Victor  added the comment:

My PR 4502 implements all optimizations, but also converts warnings.filters to 
a tuple, to detect when detect attempts to modify directly warnings.filters, 
rather than using function calls.

Problem: test.libregrtest module uses a pattern like this:

  old_filters = warnings.filters[:]  # in function 1
  (...)
  warnings.filters[:] = old_filters  # in function 2

While this pattern is perfectly fine when filters is a list, 
"warnings.filters[:] = old_filters" fails since a tuple is immutable.

Again, the question is if it's ok to break such code. I'm no more sure that 
catch_warnings() handles completly this use case, and a context manager is not 
convenient in the specific case of test.libregrest.

--

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

STINNER Victor  added the comment:


New changeset 82656276caf4cb889193572d2d14dbc5f3d2bdff by Victor Stinner in 
branch 'master':
bpo-27535: Optimize warnings.warn() (#4508)
https://github.com/python/cpython/commit/82656276caf4cb889193572d2d14dbc5f3d2bdff


--

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +4445

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

STINNER Victor  added the comment:

I published my WIP work on optimizing warnings: PR 4502, it's based on PR 4489. 
The code should be cleaned up, especially the "_PyRuntime" part. The default 
action caching should be fixed.

--

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +4440

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

STINNER Victor  added the comment:

I implemented a cache for warnings.filters in C. With my WIP patch which 
doesn't touch the registry for the ignore action, warnings.warn() is now faster 
than the current code ;-)

haypo@selma$ ./python -m perf compare_to ref.json patch.json 
Mean +- std dev: [ref] 938 ns +- 72 ns -> [patch] 843 ns +- 57 ns: 1.11x faster 
(-10%)

There is a single test in test_warnings which modifies directly 
warnings.filters:

@support.cpython_only
def test_issue31416(self):
# warn_explicit() shouldn't cause an assertion failure in case of a
# bad warnings.filters or warnings.defaultaction.
wmod = self.module
with original_warnings.catch_warnings(module=wmod):
wmod.filters = [(None, None, Warning, None, 0)]
with self.assertRaises(TypeError):
wmod.warn_explicit('foo', Warning, 'bar', 1)

wmod.filters = []
with support.swap_attr(wmod, 'defaultaction', None), \
 self.assertRaises(TypeError):
wmod.warn_explicit('foo', Warning, 'bar', 1)

I don't think that it's common to modify warnings.filters directly. Maybe we 
can make this sequence immutable in the public API to prevent misuse of the 
warnings API? To force users to use warnings.simplefilter() and 
warnings.filterwarnings()?

IMHO the main usage of modifying directyl warnings.filters it to save/restore 
filters. But there is already an helper for that: warnings.catch_warnings().

--

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

STINNER Victor  added the comment:

The performance bottleneck of warnings.warn() is the dance between the C 
_warnings module and the Python warnings module. The C code retrieves many 
attributes, like warnings.filters, at each call, and does conversion from 
Python objects to C objects.

There is already a mecanism to invalidate a "cache" in the C module: 
warnings._filters_mutated() is called by warnings.filterwarnings() for example.

Maybe the C module could convert all filters at once into an efficient C 
structure, but throw this away once on cache invalidation.

The problem is that I'm not sure that it's ok to implement such deeper cache at 
C level. Is it part of the warnings "semantics" to allow users to modify 
directly warnings.filters? Must the C module always lookup in sys.modules if 
the 'warnings' modules didn't change?

Outside test_warnings, do you know an use case where lookup for the 'warnings' 
module and 'warnings.filters' must be done at *each* warnings.warn() call?

--

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

STINNER Victor  added the comment:

> $ ./python -m perf compare_to ref.json patch.json 
> Mean +- std dev: [ref] 903 ns +- 70 ns -> [patch] 1.06 us +- 0.06 us: 1.17x 
> slower (+17%)

We are talking about a difference of 157 nanoseconds. On the same laptop, a 
Python function call which does nothing already takes 76.6 ns +- 4.1 ns. So the 
overhead of my PR 4489 is the cost of two Python function calls which do 
nothing. Do you think that it's an inacceptable overhead?

--

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

I think it is common if filters don't depend on the warning test. In this case 
we can test and set the key (None, category, lineno) before (text, category, 
lineno).

--

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[Nick Coghlan]

Nick Coghlan  added the comment:

Touching the filesystem implicitly can have all sorts of unintentional side 
effects (especially in read-only environments), so we strongly prefer not to do 
that.

The most general solution here would be to turn the warnings registry into an 
LRU cache - short-lived applications wouldn't see any changes, while long-lived 
applications with changing warning messages would hit the upper bound and then 
stay there.

I also like Victor's change to have the "ignore" option skip touching the 
registry. If we decide we still want to optimise out the linear filter scan for 
those cases, then I'd suggest lazily building a dispatch table with separate 
per-category warnings filter lists (updating the dispatch table on the next 
emitted warning after the filter list changes), and then only falling back to 
scanning the full filter list for warning categories that aren't an exact match 
for anything in the dispatch table.

--
nosy: +ncoghlan

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[Decorater]

Decorater  added the comment:

If it was me I would store the warning registry in an error log or something in 
the current directory that ran python itself. (maybe something like ``[main 
script name].log``? This way it generates the warnings like usual and does not 
eat up memory. And then the log could to turned off when they dont want any 
warnings (when ignored).

--
nosy: +Decorater

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

STINNER Victor  added the comment:

Attached bench_ignore_warn.py: microbenchmark (using the perf module) to 
measure the performance of emitting a warning when the warning is ignored.

I added two basic optimizations to my PR 4489. With these optimizations, the 
slowdown is +17% on such microbenchmark:

$ ./python -m perf compare_to ref.json patch.json 
Mean +- std dev: [ref] 903 ns +- 70 ns -> [patch] 1.06 us +- 0.06 us: 1.17x 
slower (+17%)

The slowdown was much larger without optimizations, +42%:

$ ./python -m perf compare_to ref.json ignore.json 
Mean +- std dev: [ref] 881 ns +- 59 ns -> [ignore] 1.25 us +- 0.08 us: 1.42x 
slower (+42%)


About the memory vs CPU tradeoff, we are talking around ~1000 ns. IMHO 1000 ns 
is "cheap" (fast) and emitting warnings is a rare event Python. I prefer to 
make warnings slower than "leaking" memory (current behaviour: warnings 
registry growing with no limit).

--
Added file: https://bugs.python.org/file47282/bench_ignore_warn.py

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

> While I don't see how we can avoid "leaking memory" (growing the registry) 
> for actions like "once", I think that it's ok to don't touch the registry for 
> the "ignore" action. So at least, an application ignoring ResourceWarning 
> will not leak anymore.

This is a caching for speed. If the same line of code emits the same warning, 
it is enough to tests the registry and don't apply all warning filters.

I think we should find better compromise between performance if all messages 
equal and memory usage if all messages are different. Since caching for the 
"ignore" action affects only performance, not semantic, we could check whether 
there are too much warnings for the "ignore" action are cached in the same 
line. The problem is how to make this effectively. And what is "too much".

--

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[Jonathan Bastien-Filiatrault]

Jonathan Bastien-Filiatrault  added the comment:

> But it will still "leak" when you display ResourceWarning warnings with an 
> action different than "always". In this case, IMHO the root issue is more the 
> code which doesn't close the resource, than Python itself.

Not closing a file is a bug, but under normal circumstances it causes no leak 
by itself. The fact that the warnings module leaks in this case seems a 
problem. Had I logged warnings correctly, I would have found the bug by looking 
at the application log rather than by investigating the cause of the OOM killer 
invocation.

IMHO, the warnings module should have upper bounds on memory consumption to 
avoid DOSing itself.

--

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

STINNER Victor  added the comment:

> The best way to fix this is excluding file name from warning message. As a 
> result, the message for every file will be the same and warnings registry 
> will not grow.

Your warning comes from the io module which uses the message: "unclosed file 
%r" % file.

I dislike the idea of removing the filename from this warning message, since 
the filename is very useful to identify where the bug comes from.

Different things are discussed here:

* First of all, if an application is correctly written, ResourceWarning is not 
emitted, and so Python doesn't leak memory

* Using the "always" action, ResourceWarning doesn't leak neither.

* Using a different action like "default", ResourceWarning does leak memory in 
hidden warning registries.

While I don't see how we can avoid "leaking memory" (growing the registry) for 
actions like "once", I think that it's ok to don't touch the registry for the 
"ignore" action. So at least, an application ignoring ResourceWarning will not 
leak anymore.

But it will still "leak" when you display ResourceWarning warnings with an 
action different than "always". In this case, IMHO the root issue is more the 
code which doesn't close the resource, than Python itself.

--

___
Python tracker 

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

[issue27535] Ignored ResourceWarning warnings leak memory in warnings registries

[STINNER Victor]

Change by STINNER Victor :


--
title: Memory leaks when opening tons of files -> Ignored ResourceWarning 
warnings leak memory in warnings registries

___
Python tracker 

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