[issue22166] test_codecs leaks references

[STINNER Victor]

STINNER Victor  added the comment:

Codec tests don't leak anymore. I lost track of this very old issue. I don't 
think that anything should be done. Python code base changed a lot since 2014. 
I close the issue.

--
resolution:  -> fixed
stage: needs patch -> 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

[issue22166] test_codecs leaks references

[Stéphane Wirtel]

Stéphane Wirtel  added the comment:

For 3.7 and 3.8, I have just tried with this command to find leaks in the 
test_codec* but nothing :/

./python -m test -v -l -R 6:6 -u all \
test_codecencodings_iso2022 \
test_codecs \
test_codecmaps_hk test_codecmaps_tw \
test_codecencodings_tw \
test_codecencodings_cn \
test_codeccallbacks \
test_codecencodings_hk \
test_codecencodings_jp \
test_codecencodings_kr \
test_codecmaps_jp \
test_codecmaps_cn \
test_codecmaps_kr

If you have another technic for the detection of leaks, please inform me but I 
think this issue could be closed maybe we have fixed the issue with time.

--
nosy: +matrixise

___
Python tracker 

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

[issue22166] test_codecs leaks references

[Brett Cannon]

Change by Brett Cannon :


--
nosy:  -brett.cannon

___
Python tracker 

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

[issue22166] test_codecs leaks references

[Serhiy Storchaka]

Change by Serhiy Storchaka :


--
assignee:  -> serhiy.storchaka

___
Python tracker 

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

[issue22166] test_codecs leaks references

[STINNER Victor]

STINNER Victor  added the comment:

> Do you mind to create a PR Victor?

I completely forgot this issue and I don't plan to fix it in the short-term. 
Feel free to write PR if you want.

--

___
Python tracker 

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

[issue22166] test_codecs leaks references

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

Do you mind to create a PR Victor?

--

___
Python tracker 

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

[issue22166] test_codecs leaks references

[Serhiy Storchaka]

Change by Serhiy Storchaka :


--
versions: +Python 3.7, Python 3.8 -Python 3.4, Python 3.5

___
Python tracker 

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

[issue22166] test_codecs leaks references

[Nick Coghlan]

Nick Coghlan added the comment:

Victor, would you be willing to take over pushing this one forward? I believe 
you have a much better idea of what's still needed than I do.

--
assignee: ncoghlan -> 

___
Python tracker 

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

[issue22166] test_codecs leaks references

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


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

[issue22166] test_codecs leaks references

[Nick Coghlan]

Changes by Nick Coghlan :


--
assignee:  -> ncoghlan

___
Python tracker 

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

[issue22166] test_codecs leaks references

[Marc-Andre Lemburg]

Marc-Andre Lemburg added the comment:

On 15.09.2014 15:19, STINNER Victor wrote:
> 
> STINNER Victor added the comment:
> 
> IMO test_codecs_fix1.patch is still needed.
> 
> Review of Nick's change:
> 
> +interp = PyThreadState_GET()->interp;
> +if (interp->codec_search_path == NULL) {
> +return -1;
> +}
> 
> I don't think that this line raises an exception.

Agreed.

Please also add some documentation to the codecs.h file and
perhaps a test for the function itself ;-)

--

___
Python tracker 

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

[issue22166] test_codecs leaks references

[STINNER Victor]

STINNER Victor added the comment:

IMO test_codecs_fix1.patch is still needed.

Review of Nick's change:

+interp = PyThreadState_GET()->interp;
+if (interp->codec_search_path == NULL) {
+return -1;
+}

I don't think that this line raises an exception. You should use an assertion 
here (or raises an exception, but it would be overkill).

+encodings._cache.pop(self.codec_name, None)
+try:
+_forget_codec(self.codec_name)
+except KeyError:
+pass

You may move these lines to a private encoding._forget() function.

By the way, codecs._forget() should maybe catch and ignore KeyError?

>  _TEST_CODECS.pop(self.codec_name, None)

This line may be moved to set_codec() using self.addCleanup(). (Well, it's not 
directly related to your change.)

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

[issue22166] test_codecs leaks references

[Nick Coghlan]

Changes by Nick Coghlan :


--
resolution:  -> fixed
stage:  -> 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

[issue22166] test_codecs leaks references

[Roundup Robot]

Roundup Robot added the comment:

New changeset fcf45ec7863e by Nick Coghlan in branch '3.4':
Issue #22166: clear codec caches in test_codecs
https://hg.python.org/cpython/rev/fcf45ec7863e

New changeset 322ee2f2e922 by Nick Coghlan in branch 'default':
Merge fix for issue #22166 from 3.4
https://hg.python.org/cpython/rev/322ee2f2e922

--
nosy: +python-dev

___
Python tracker 

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

[issue22166] test_codecs leaks references

[Nick Coghlan]

Nick Coghlan added the comment:

Targeted API to forget a particular codec name.

Normally we don't care about the caching (as evidenced by the fact it took 14+ 
years to want this feature).

The intent with this API is to be able to selective purge a particular codec 
when we either care about getting the latest version of a specific codec (no 
current use cases) or to avoid the appearance of leaking references when 
looking up lots of custom codecs (this case).

I considered dropping the per-test naming scheme, but decided I liked the fact 
it kept these test instances orthogonal, even though they all use the shared 
TEST_CODECS registry. The lack of global side effects is also a benefit of just 
popping the named codec, rather than purging the entire cache.

--
Added file: 
http://bugs.python.org/file36563/issue22166_with_forget_codec_helper_api.diff

___
Python tracker 

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

[issue22166] test_codecs leaks references

[Antoine Pitrou]

Antoine Pitrou added the comment:

Well, the point here is to add a private API just for our test suite, not 
something users would want to call.

--

___
Python tracker 

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

[issue22166] test_codecs leaks references

[Marc-Andre Lemburg]

Marc-Andre Lemburg added the comment:

On 06.09.2014 21:14, Nick Coghlan wrote:
> 
> This investigation also showed why the original tests that used a
> non-unique name didn't work: the codec was getting cached the first time
> through and hence not seeing the different codecs registered by other tests.
> 
> MAL, my plan was to provide an API to forget a specific named codec rather
> than flush the whole cache. I agree a private C API would also be a good
> idea, though.

I don't think such a specific API for only forgetting a single codec
is useful outside this particular test case, since you typically
don't know the names of the cached codecs.

--

___
Python tracker 

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

[issue22166] test_codecs leaks references

[Nick Coghlan]

Nick Coghlan added the comment:

This investigation also showed why the original tests that used a
non-unique name didn't work: the codec was getting cached the first time
through and hence not seeing the different codecs registered by other tests.

MAL, my plan was to provide an API to forget a specific named codec rather
than flush the whole cache. I agree a private C API would also be a good
idea, though.

--

___
Python tracker 

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

[issue22166] test_codecs leaks references

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

May be just do not use the unique name?

--
nosy: +serhiy.storchaka

___
Python tracker 

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

[issue22166] test_codecs leaks references

[Marc-Andre Lemburg]

Marc-Andre Lemburg added the comment:

On 06.09.2014 15:03, Nick Coghlan wrote:
> A "codecs._forget_codec" helper in the C module would be all that was needed 
> to handle it. Other implementations could then also use that to clear their 
> own internal cache (if they have one).

If you want to take that approach, please call that function
codecs._clear_lookup_cache() and also add a new C API
_PyCodecRegistry_ClearLookupCache() to Python/codecs.c.

Thanks,
-- 
Marc-Andre Lemburg
eGenix.com

--
nosy: +lemburg

___
Python tracker 

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

[issue22166] test_codecs leaks references

[Nick Coghlan]

Nick Coghlan added the comment:

Yeah, I'm wary of the GC hack as well. It's a nice way to prove that this the 
internal cache is the problem, but probably not something we want to commit.

A "codecs._forget_codec" helper in the C module would be all that was needed to 
handle it. Other implementations could then also use that to clear their own 
internal cache (if they have one).

--

___
Python tracker 

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

[issue22166] test_codecs leaks references

[Antoine Pitrou]

Antoine Pitrou added the comment:

I'd much rather see a private function somewhere (in the _codecs module) to 
clear that private interpreter cache.

--

___
Python tracker 

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

[issue22166] test_codecs leaks references

[Nick Coghlan]

Nick Coghlan added the comment:

Yep, it looks like the mess with the reference cycles in the tracebacks was 
just a secondary problem: the real culprit was the internal cache keeping 
everything else alive.

Attached patch just adds clearing encodings._cache and the CPython internal 
cache to the ExceptionChainingTest.tearDown() method.

The latter is accomplised via gc.get_referrers() and popping the codec's name 
from any dict remaining in that list after clearing the known caches, so it 
will actually handle any hidden name based caches on any interpreter.

--
Added file: 
http://bugs.python.org/file36560/issue22166_clear_internal_cache.diff

___
Python tracker 

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

[issue22166] test_codecs leaks references

[Nick Coghlan]

Nick Coghlan added the comment:

Ah, I think I've found a possible underlying culprit: there's a separate C 
level "codec_search_cache" on the interpreter object that is independent of 
encodings._cache.

The attached patch eliminates the reference leaks entirely (using 
gc.get_referrers() to get a reference to the otherwise inaccessible internal 
interpreter cache).

However, the patch has a bunch of changes that may not be necessary if that 
cache is dealt with properly, so I'm going to revert everything, and *start* 
with fixing the cache cleanup.

--
Added file: 
http://bugs.python.org/file36559/issue22166_no_leaks_too_many_changes.diff

___
Python tracker 

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

[issue22166] test_codecs leaks references

[Nick Coghlan]

Nick Coghlan added the comment:

Yeah, the unique name was a hack to deal with the fact I couldn't unregister 
anything.

As a simpler short term fix for the reference leak, perhaps it would be worth 
trying a call to "traceback.clear_frames(caught)" as a new final line in 
ExceptionChainingTest.assertWrapped?

--

___
Python tracker 

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

[issue22166] test_codecs leaks references

[Antoine Pitrou]

Antoine Pitrou added the comment:

> We should maybe enhance the codecs API to be able to unregister a codec?

That would be nice. At least as a private API in 3.4.

--
nosy: +pitrou

___
Python tracker 

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

[issue22166] test_codecs leaks references

[STINNER Victor]

STINNER Victor added the comment:

ExceptionChainingTest creates a random codec name. If you change the codec name 
to a fixed string like "xxx", ExceptionChainingTest.test_raise_by_type() 
doesn't leak anymore (when  test_codecs_fix1.patch is applied), but other tests 
start to fail.

We should maybe enhance the codecs API to be able to unregister a codec?

--

___
Python tracker 

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

[issue22166] test_codecs leaks references

[STINNER Victor]

STINNER Victor added the comment:

Attached: Draft patch to fix some reference leaks in test_codecs.

--
title: test_codecs "leaking" references -> test_codecs leaks references
Added file: http://bugs.python.org/file36524/test_codecs_fix1.patch

___
Python tracker 

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