test test_multibytecodec: Test_IncrementalEncoder.test_subinterp() leaks references

[vstinner]

BPO	42866
Nosy	@vstinner, @corona10, @miss-islington, @erlend-aasland
PRs	bpo-42866: Fix refleak in CJK getcodec() #24165 bpo-42866: Add traverse func to _multibytecodec.MultibyteCodec #24166 [3.9] bpo-42866: Fix refleak in CJK getcodec() (GH-24165) #24167 [3.8] bpo-42866: Fix refleak in CJK getcodec() (GH-24165) #24168

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2021-01-08.14:49:29.209>
created_at = <Date 2021-01-08.09:28:39.500>
labels = ['library', '3.10']
title = 'test test_multibytecodec: Test_IncrementalEncoder.test_subinterp() leaks references'
updated_at = <Date 2021-01-08.14:49:29.209>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2021-01-08.14:49:29.209>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2021-01-08.14:49:29.209>
closer = 'vstinner'
components = ['Library (Lib)']
creation = <Date 2021-01-08.09:28:39.500>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 42866
keywords = ['patch']
message_count = 18.0
messages = ['384643', '384645', '384647', '384648', '384650', '384651', '384652', '384653', '384654', '384661', '384662', '384665', '384667', '384669', '384670', '384671', '384673', '384675']
nosy_count = 4.0
nosy_names = ['vstinner', 'corona10', 'miss-islington', 'erlendaasland']
pr_nums = ['24165', '24166', '24167', '24168']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue42866'
versions = ['Python 3.10']

[vstinner]

$ ./python -m test test_multibytecodec -m test.test_multibytecodec.Test_IncrementalEncoder.test_subinterp -R 3:3
(...)
test_multibytecodec leaked [258, 258, 258] references, sum=774

I simplified the code. The following test leaks references:

    def test_subinterp(self):
        import _testcapi
        code = textwrap.dedent("""
            import _codecs_jp
            codec = _codecs_jp.getcodec('cp932')
            codec = None
        """)
        _testcapi.run_in_subinterp(code)

_codecs_jp.getcodec() is defined in Modules/cjkcodecs/cjkcodecs.h. Extract:

    cofunc = getmultibytecodec();
    ...
    codecobj = PyCapsule_New((void *)codec, PyMultibyteCodec_CAPSULE_NAME, NULL);
    if (codecobj == NULL)
        return NULL;

    r = PyObject_CallOneArg(cofunc, codecobj);
    Py_DECREF(codecobj);

getmultibytecodec() is the _multibytecodec.__create_codec() which is defined in Modules/cjkcodecs/multibytecodec.c. Simplified code:

    codec = PyCapsule_GetPointer(arg, PyMultibyteCodec_CAPSULE_NAME);
    _multibytecodec_state *state = _multibytecodec_get_state(module);
    self = PyObject_New(MultibyteCodecObject, state->multibytecodec_type);
    self->codec = codec;
    return (PyObject *)self;

[vstinner]

I added the test which leaks in bpo-42846.

commit 07f2cee
Author: Victor Stinner <vstinner@python.org>
Date: Fri Jan 8 00:15:22 2021 +0100

bpo-42846: Convert CJK codec extensions to multiphase init (GH-24157)

Convert the 6 CJK codec extension modules (_codecs_cn, _codecs_hk,
_codecs_iso2022, _codecs_jp, _codecs_kr and _codecs_tw) to the
multiphase initialization API (PEP-489).

Remove getmultibytecodec() local cache: always import
_multibytecodec. It should be uncommon to get a codec. For example,
this function is only called once per CJK codec module.

Fix a reference leak in register_maps() error path.

I don't think that the leak is new. It's just that it wasn't seen previously.

[vstinner]

By the way, I wrote an article "Leaks discovered by subinterpreters":
https://vstinner.github.io/subinterpreter-leaks.html

This leak may be new kind related to capsule, I'm not sure so far.

[erlend-aasland]

Thank you so much for taking the time to write these blog posts, Victor, and for explaining your fixes is such great detail. It is very helpful!

[vstinner]

PR 24165 fix one reference leak in the getcodec() function of CJK codecs. But it doesn't fix all ref leaks of this issue.

[vstinner]

The leak can be simplified to:

    @support.cpython_only
    def test_subinterp(self):
        import _testcapi
        code = textwrap.dedent("""
            import encodings
            import _codecs_jp
            encodings._cache['cp932'] = _codecs_jp.getcodec('cp932')
        """)
        res = _testcapi.run_in_subinterp(code)
        self.assertEqual(res, 0)

[vstinner]

Links about ref leaks related to encodings:

• https://bugs.python.org/issue1635741#msg364833
• https://bugs.python.org/issue1635741#msg364968
• https://bugs.python.org/issue36854#msg357160
• master...phsilva:remove-codec-caches

[vstinner]

See also bpo-42671 "Make the Python finalization more deterministic" but it seems like PR 23826 makes the leak worse (+2000 leaked references instead of +200) :-)

[vstinner]

encodings._cache['cp932'] = _codecs_jp.getcodec('cp932')

• encodings._cache is kept alive by encodings.search_function.__globals__
• encodings.search_function function is kept alive by PyInterpreterState.codec_search_path list. The function by _PyCodec_Register() in encodings/init.py: codecs.register(search_function).

For example, unregistering the search function prevents the leak:

            import encodings
            import _codecs_jp
            encodings._cache['cp932'] = _codecs_jp.getcodec('cp932')

            import codecs
            codecs.unregister(encodings.search_function)

The PyInterpreterState.codec_search_path list is cleared at Python exit by interpreter_clear().

The weird part is that the _codecs_jp.getcodec('cp932') codec object *is* deleted. I checked and multibytecodec_dealloc() is called with the object stored in the encodings cache.

A _multibytecodec.MultibyteCodec instance (MultibyteCodecObject* structure in C) is a simple type: it only stores pointer to C functions and C strings. It doesn't contain any Python object. So I don't see how it could be part of a reference cycle by itself. Moreover, again, it is deleted.

[vstinner]

Calling gc.collect() twice works around the issue, which sounds like a missing traverse function somewhere.

diff --git a/Python/pystate.c b/Python/pystate.c
index c791b23999..66bbe1bf7d 100644
--- a/Python/pystate.c
+++ b/Python/pystate.c
@@ -321,6 +321,7 @@ interpreter_clear(PyInterpreterState *interp, PyThreadState *tstate)
 
     /* Last garbage collection on this interpreter */
     _PyGC_CollectNoFail(tstate);
+    _PyGC_CollectNoFail(tstate);
     _PyGC_Fini(tstate);
 
     /* We don't clear sysdict and builtins until the end of this function.

[vstinner]

New changeset e542d41 by Victor Stinner in branch 'master':
bpo-42866: Fix refleak in CJK getcodec() (GH-24165)
e542d41