Clear static types in Py_Finalize() for embedded Python #90575
Comments
|
Converting static types to heap types is a work-in-progress:
Static types are still causing issues with Py_Initialize() / Py_Finalize() is called multiple times in the same process, which is a supported use case for embedded Python. See bpo-45691 "Partial moving of core objects to interpreter state is incorrect at best, unsafe at worse" for a recent example with sys.float_info where sys.float_info.n_unnamed_fields holds a reference to a Python object. In june 2020, I wrote #64962 to clear static types at exit. But I abandoned my attempt because of bugs. Copy of my https://bugs.python.org/issue1635741#msg371119 message: """ It doesn't look to be safe to clear static types. Many functions rely on the fact that static types are "always there" and are never finalized. Also, only a few static types are cleared by my PR: many static types are left unchanged. For example, static types of the _io module. It seems like a safer approach is to continue the work on bpo-40077: "Convert static types to PyType_FromSpec()". I propose a "best effort" approach: only clear a static type if it is no longer used. For example, check its reference count and its __subclasses__() method. |
|
If we have static types, that means there is a mechanism to share some objects across interpreters. |
Sharing objects between interpreters is bad and is causing complex bugs. See a recent example of an object traveling from one interpreter to another and then causing a random crash on Windows related to the garbage collector:
I would prefer to discuss that in other issues like bpo-40255 or bpo-39511, and focus this issue on fixing the static types implementation when Python is embedded in an application. -- Hum, this issue is not really related to sub-interpreters. My proposed PR only changes Py_Finalize(): the main interpreter. |
vstinner
changed the title
vstinner
changed the title
vstinner
added
interpreter-core
That's your opinion, I don't necessarily share it.
But converting static things (types, small ints) to heap is also causing bugs :( ---- Anyway, for this issue: is there a way to test if the types are correctly reinitialized if there are multiple finalize/init cycles? |
|
If tomorrow static types are shared between sub-interpreters, it doesn't solve this problem: we still need to release memory allocated by Py_Initialize() in Py_Finalize() when Python is embedded. This issue is a sub-set of the big bpo-1635741 which explains the rationale. Again, this issue is not about sub-interpreters.
I wasn't sure how to test it. But well, Dong-hee and you asked for tests, so I added tests for my PR ;-) |
|
I checked with Valgrind the affect of PR 30645. main branch: ==330902== LEAK SUMMARY: With the PR: ==332161== LEAK SUMMARY: It frees 21,064 bytes (20 kB) in Py_Finalize(). |
|
Another measure using the command: PYTHONHASHSEED=0 ./python -X showrefcount -c passI had to run the command 20 times to get a stable value, I don't know why. main branch: [21981 refs, 5716 blocks] => the PR removes 94 references and 49 memory blocks. |
|
See also bpo-46449 "Deep-freezed modules create inconsistency in sys.gettotalrefcount() (_Py_Reftotal)". |
Oh, test_unittest crashed on s390x Debian 3.x: I fail to see the relationship between my change and this crash. I don't expect test_unittest to call Py_Finalize() and then Py_Initialize() again. --- Current thread 0x000003ff89cf8720 (most recent call first): Extension modules: _testcapi (total: 1) |
|
Maybe the problem is that I changed the order in which types are initialized in _PyTypes_InitTypes()? So far, test_unittest crashed on 4 buildbot workers:
|
Ok, this change fixed buildbots. I saw code in typeobject.c which uses a borrowed reference to tp_subclasses with a loop which can modify tp_subclasses. This code should be modified to hold a strong reference to tp_subclasses while accessing it. The test_mock_add_spec() test of test_unittest modifies the subclasses of many types and so is indirectly a stress tests for code accessing tp_subclasses. That's why the regression was only seen in this specific test. |
|
Kumar Aditya: "The following patch further reduces the reference but not sure if it is correct (...)" Right! PyContext and PyHamt types were on my TODO list ;-) They are now cleared since this change: bpo-46417: Clear more static types (GH-30796) It's similar to your change, but more complete :-) |
|
Attached cannot_deallocate.patch explains why some static types cannot be deallocated. It lists (static) types which are not cleard properly at Python exit. Simplest example: $ ./python -c pass
Cannot clear type 'object': it still has subclasses
* EncodingMap
* fieldnameiterator
* formatteriterator
* BaseException
* _io._IOBase
* _io._BytesIOBuffer
* _io.IncrementalNewlineDecoderMore complete example: $ ./python setup.py build
running build
(...)
Cannot clear type 'types.GenericAlias': it still has subclasses
* _CallableGenericAlias
Cannot clear type 'tuple': it still has subclasses
* datetime.IsoCalendarDate
* DecimalTuple
Cannot clear type 'staticmethod': it still has subclasses
* abstractstaticmethod
Cannot clear type 'property': it still has subclasses
* abstractproperty
Cannot clear type 'list': it still has subclasses
* MyList
Cannot clear type 'dict': it still has subclasses
* collections.defaultdict
* StgDict
Cannot clear type 'classmethod': it still has subclasses
* abstractclassmethod
Cannot clear type 'type': it still has subclasses
* ABCMeta
* _ctypes.PyCStructType
* _ctypes.UnionType
* _ctypes.PyCPointerType
* _ctypes.PyCArrayType
* _ctypes.PyCSimpleType
* _ctypes.PyCFuncPtrType
Cannot clear type 'object': it still has subclasses
* type
* classmethod
* dict
* list
* property
* staticmethod
* tuple
* types.GenericAlias
* EncodingMap
* fieldnameiterator
* formatteriterator
* BaseException
* ModuleSpec
* FrozenImporter
* _io._IOBase
* _io._BytesIOBuffer
* _io.IncrementalNewlineDecoder
* _LoaderBasics
* FileLoader
* _abc._abc_data
* ABC
* Hashable
* Awaitable
* AsyncIterable
* Iterable
* Sized
* Container
* Callable
* itertools.accumulate
* itertools.combinations
* itertools.combinations_with_replacement
* itertools.cycle
* itertools.dropwhile
* itertools.takewhile
* itertools.islice
* itertools.starmap
* itertools.chain
* itertools.compress
* itertools.filterfalse
* itertools.count
* itertools.zip_longest
* itertools.pairwise
* itertools.permutations
* itertools.product
* itertools.repeat
* itertools.groupby
* itertools._grouper
* itertools._tee
* itertools._tee_dataobject
* collections.deque
* _collections._deque_iterator
* _collections._deque_reverse_iterator
* _collections._tuplegetter
* _IterationGuard
* WeakSet
* _struct.Struct
* _struct.unpack_iterator
* datetime.date
* datetime.time
* datetime.timedelta
* datetime.tzinfo
* _pickle.Pdata
* _pickle.PicklerMemoProxy
* _pickle.UnpicklerMemoProxy
* _pickle.Pickler
* _pickle.Unpickler
* _socket.socket
* _asyncio.FutureIter
* TaskStepMethWrapper
* _RunningLoopHolder
* _asyncio.Future
* _xxsubinterpreters.ChannelID
* matmulType
* ipowType
* awaitType
* MethodDescriptorBase
* GenericAlias
* Generic
* MethInstance
* MethClass
* MethStatic
* _testcapi.HeapCType
* _testcapi.ContainerNoGC
* _curses.window
* ossaudiodev.oss_audio_device
* ossaudiodev.oss_mixer_device
* _elementtree._element_iterator
* xml.etree.ElementTree.TreeBuilder
* xml.etree.ElementTree.Element
* xml.etree.ElementTree.XMLParser
* decimal.Decimal
* decimal.Context
* decimal.SignalDictMixin
* decimal.ContextManager
* Number
* _multiprocessing.SemLock
* xxlimited.Xxo
* CArgObject
* _ctypes.CThunkObject
* _ctypes._CData
* _ctypes.CField
* _ctypes.DictRemover
* _ctypes.StructParam_Type |
|
The _PyStaticType_Dealloc() function cannot call PyObject_ClearWeakRefs() if the refcount is not zero. I am not sure if it's a real issue or not. Maybe the weakref list must be cleared (release memory), but callbacks must not be called? |
|
cannot_deallocate2.patch: updated patch to debug which types are not cleared at exit. |
At this commit, Py_Finalize() no longer leaks references. It exceeded my expectations, it even makes _Py_RefTotal negative :-D $ ./python -I -X showrefcount -c pass
[-4 refs, 61 blocks]I'm not sure why it's negative, but bpo-46449 should be investigate first. |
|
See also bpo-46476: Not all memory allocated by _Py_Quicken() is released at Python exit. |
If you apply my workaround for bpo-46476: Python no longer leaks any memory block at exit for the simplest command! $ ./python -I -X showrefcount -c pass
[-5 refs, 0 blocks]Moreover, I modified deepfreeze to only freeze importlib._bootstrap and importlib._bootstrap_external. It confirms that bpo-46449 is causing the negative reference count, because with these additional local changes I get a positive _Py_RefTotal: $ ./python -I -X showrefcount -c pass
[6 refs, 0 blocks] |
|
I close the issue. I cleared most static types at exit. Following work can be done in bpo-40077 or other issues. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: