Skip to content

Fix weakref bug preventing garbage collection of template-based models#14

Open
kmaziarz wants to merge 2 commits intomainfrom
kmaziarz/fix-weakref-bug
Open

Fix weakref bug preventing garbage collection of template-based models#14
kmaziarz wants to merge 2 commits intomainfrom
kmaziarz/fix-weakref-bug

Conversation

@kmaziarz
Copy link
Copy Markdown
Contributor

@kmaziarz kmaziarz commented Apr 8, 2026

Our template-based models rely on RuleApplicationServer, which spins up parallel processes for template application. When the server is garbage-collected (for example, because one calls del on the enclosing model), those processes should be terminated, and we tried to ensure this by using weakref.finalize to register self.close to be called. However, this has a fundamental bug: self.close contains a reference to self, thus a reference to self will exist even after doing del on the model, so the server can never be garbage collected; the processes are only killed when Python itself shuts down.

This PR fixes the bug by moving the cleanup logic to a staticmethod instead. It also adds a test to verify the processes are cleaned up; before the bugfix commit the test does fail.

For a minimal illustration of the bug, compare

import weakref

class Server:
    def close(self):
        print("close() called")

    def __init__(self):
        weakref.finalize(self, self.close)

server = Server()

print("before del")
del server
print("after del")

# close is called at the end of script

with the fixed version relying on a static method

import weakref

class Server:
    @staticmethod
    def static_close():
        print(f"static_close() called")

    def __init__(self):
        weakref.finalize(self, Server.static_close)

server = Server()

print("before del")
del server  # close is immediately called here
print("after del")

We did not find this issue before, as in most cases we load a single model, use it throughout some workflow, and then the script ends. The issue manifests itself when one uses a few models within one script, and calls del in between in an attempt to free memory. With the changes here, this now works as expected (i.e. memory used by the model is recovered on del).

@kmaziarz kmaziarz requested review from jla-gardner and mrwnmsr April 8, 2026 14:01
@kmaziarz kmaziarz force-pushed the kmaziarz/fix-weakref-bug branch from 7061ce4 to d97b267 Compare April 10, 2026 13:37
@kmaziarz kmaziarz requested a review from fiberleif April 13, 2026 13:24
Copy link
Copy Markdown

@fiberleif fiberleif left a comment

Choose a reason for hiding this comment

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

LGTM

nit: It looks like _connection_try_close is no longer used after the new static method _finalize_server. Do we want to remove it as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants