C API/GC: datetime.datetime doesn't have `Py_TPFLAGS_HAVE_GC`, potentially causing leaks in `tzinfo` cycles

[ariebovenberg]

Bug report

Bug description:

The GC docs say (emphasis mine)

Python’s support for detecting and collecting garbage which involves circular references requires support from object types which are “containers” for other objects which may also be containers. Types which do not store references to other objects, or which only store references to atomic types (such as numbers or strings), do not need to provide any explicit support for garbage collection.

However in datetime, PyDateTime_DateTimeType is a container (of tzinfo), but doesn’t have the GC flags set.

With help on Python's discuss forums, I was able to get to this minimal example that appears to show a possible leak.

from datetime import datetime, tzinfo
import gc, sys

class MyTzinfo(tzinfo): pass

tz = MyTzinfo()
dt = datetime(2000, 1, 1, tzinfo=tz)
tz.foo = dt  # circular ref introduced here
assert sys.getrefcount(tz) == 3 # 1 from dt, 1 variable, 1extra from passing it in
assert sys.getrefcount(tz) == 3 # 1 from tz, 1 variable, 1extra from passing it in
assert len([obj for obj in gc.get_objects() if isinstance(obj, MyTzinfo)]) == 1
del tz
assert len([obj for obj in gc.get_objects() if isinstance(obj, MyTzinfo)]) == 1
del dt
gc.collect()
# At this point, dt and tz should be gone
assert len([obj for obj in gc.get_objects() if isinstance(obj, MyTzinfo)]) == 0

If datetime is replaced with a dummy class, the above program works:

class datetime:
    def __init__(self, *args, **kwargs):
        self.args = args
        self.kwargs = kwargs

I'm happy to submit a PR if needed.

CPython versions tested on:

3.12

Operating systems tested on:

macOS

[pganssle]

Hopefully this doesn't actually happen in practice, since time zones shouldn't have and don't need references to datetimes, but it is possible, so we should try and fix it. I would be inclined to just let the memory leak if fixing this causes significant performance degradation (since the overwhelmingly common case is that there are no reference cycles), but if it's relatively cheap we should fix it.

[pganssle]

@ariebovenberg If you are going to make a PR, I think you need to do more than just the flags, you probably also need to implement some of the GC travesal methods as well, like tp_traverse and a few others. Feel free to make a PR.

[ariebovenberg]

Thanks, I'll definitely give it a try. Will performance automatically be checked in CI or will we need to devise a custom benchmark? I'm assuming microbenchmarks like python -m timeit don't sufficiently account for the full cost of having datetime objects GC tracked.

[pganssle]

Thanks, I'll definitely give it a try. Will performance automatically be checked in CI or will we need to devise a custom benchmark? I'm assuming microbenchmarks like python -m timeit don't sufficiently account for the full cost of having datetime objects GC tracked.

It won't be automatically checked in CI as far as I know, but I wouldn't worry about it too much, it would be nice to have some sort of sanity check benchmark, but we don't have to be too elaborate about it.

This is a correctness bug, and if it ends up being a major performance regression for someone in practice we can revisit our approach.