Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix HomeServers leaking during trial test runs #15630

Merged
merged 3 commits into from
May 19, 2023
Merged

Fix HomeServers leaking during trial test runs #15630

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3b305de
Fix `HomeServer`s leaking after tests that touch inbound federation
May 18, 2023
cc480e1
Fix `HomeServer`s leaking due to #15334
May 18, 2023
8e94932
Add newsfile
May 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/15630.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix two memory leaks in `trial` test runs.
6 changes: 5 additions & 1 deletion synapse/util/ratelimitutils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,12 @@
Iterator,
List,
Mapping,
MutableSet,
Optional,
Set,
Tuple,
)
from weakref import WeakSet

from prometheus_client.core import Counter
from typing_extensions import ContextManager
Expand Down Expand Up @@ -86,7 +88,9 @@
)


_rate_limiter_instances: Set["FederationRateLimiter"] = set()
# This must be a `WeakSet`, otherwise we indirectly hold on to entire `HomeServer`s
# during trial test runs and leak a lot of memory.
_rate_limiter_instances: MutableSet["FederationRateLimiter"] = WeakSet()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not familiar with weakset. Does this mean that if I do:

x = (1,2)
w = WeakSet()
w.insert(x)
del x

Then len(w) == 0?

Copy link
Contributor

@DMRobertson DMRobertson May 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, only certain types of objects can hold weakrefs per https://docs.python.org/3/library/weakref.html

>>> x = frozenset("abc")
>>> w.add(x)
>>> len(w)
1
>>> del x
>>> len(w)
0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's roughly the idea, yep. The set won't prevent garbage collection of any of its contents and GCed objects will magically disappear from the set.

# Protects the _rate_limiter_instances set from concurrent access
_rate_limiter_instances_lock = threading.Lock()

Expand Down
11 changes: 9 additions & 2 deletions tests/unittest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,13 +229,20 @@ def tearDown(orig: Callable[[], R]) -> R:
#
# The easiest way to do this would be to do a full GC after each test
# run, but that is very expensive. Instead, we disable GC (above) for
# the duration of the test so that we only need to run a gen-0 GC, which
# is a lot quicker.
# the duration of the test and only run a gen-0 GC, which is a lot
# quicker. This doesn't clean up everything, since the TestCase
# instance still holds references to objects created during the test,
# such as HomeServers, so we do a full GC every so often.

@around(self)
def tearDown(orig: Callable[[], R]) -> R:
ret = orig()
gc.collect(0)
# Run a full GC every 50 gen-0 GCs.
gen0_stats = gc.get_stats()[0]
gen0_collections = gen0_stats["collections"]
if gen0_collections % 50 == 0:
gc.collect()
Comment on lines +241 to +245
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any idea why we're manually kicking the GC at all here? Is this to try and make tests more deterministic somehow? (Are we relying on __del__ being called somewhere?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failed Deferreds that haven't been awaited on / consumed will error out when they are GCed (and fail the currently running test somehow). If we let the garbage collector run at its own leisure, such failures would be logged against a random future test. At least I think that's what the original code is trying to trigger.

gc.enable()
set_current_context(SENTINEL_CONTEXT)

Expand Down