T7910: Standardize vyconf session resource management - #4807
Conversation
|
❌ |
1e43226 to
d88996b
Compare
|
Rebase over current to pull in hash update for |
|
Note that darker-ruff-lint will never be happy with the auto-generated files, |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors session lifecycle management in VyOS by implementing automatic cleanup using weak references (finalizers) and improving session handling for both config-mode and op-mode sessions. The changes introduce an extant parameter to distinguish between ephemeral and persistent sessions, and add a new show_sessions API method.
Key changes:
- Automatic session cleanup using
weakref.finalize()for op-mode sessions - New
extantparameter to force persistent session lookup instead of creating new sessions - Addition of
show_sessions()method to query active sessions - Removal of manual
__del__methods in favor of finalizers
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/helpers/teardown-config-session.py | Wraps teardown logic in try-except to handle non-existent sessions gracefully |
| python/vyos/vyconf_session.py | Implements automatic session cleanup with finalizers, adds extant parameter for persistent session lookup, and adds show_sessions() method |
| python/vyos/proto/vyconf_proto.py | Adds protocol buffer definitions for the new show_sessions API |
| python/vyos/proto/vyconf_pb2.py | Updates generated protobuf code with new ShowSessions message type |
| python/vyos/configsource.py | Removes manual __del__ method as cleanup is now handled by finalizers |
| python/vyos/configsession.py | Refactors session teardown to use finalizers instead of __del__, separating cleanup logic into finalize_vyconf() method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
All Copilot suggestions resolved above (none appropriate, but for one issue of formatting in the dataclass generation script, to be handled elsewhere). |
| self.shared = shared | ||
|
|
||
| if not self.shared and self._vyconf_session: | ||
| self._finalizer = weakref.finalize( |
There was a problem hiding this comment.
From weakref.finalize documentation (https://docs.python.org/3/library/weakref.html#weakref.finalize):
Note It is important to ensure that func, args and kwargs do not own any references to obj, either directly or indirectly, since otherwise obj will never be garbage collected. In particular, func should not be a bound method of obj.
In such case object self is stored till end of script run, so it is same as using atexit. Is this intentional?.. If so, I suggest writting a comment about that.
I've checked this on simple example:
#!/usr/bin/env python3
import weakref
class C:
def __init__(self, n):
self.n = n
def finalizer(self, msg: str):
print(f"In finalizer! Storing number: {self.n}")
print(f"Final meesage: {msg}")
a = C(32)
b = C(17)
weakref.finalize(a, a.finalizer, 'a finalizer')
weakref.finalize(b, print, 'b finalizer')
print("registered")
del a
del b
print("del all")Output on my machine:
vyos ~/reviews/VD-1893$ ./test.py
registered
b finalizer
del all
In finalizer! Storing number: 32
Final meesage: a finalizerb is finalized after del, but a only at exit.
There was a problem hiding this comment.
Yes, thanks: the finalizer functions need to be class methods; I will make those changes.
There was a problem hiding this comment.
Thanks again for that review, @hedrok : an important point. The PR has been updated; your test script with analogous modifications and a sanity check would look like:
#!/usr/bin/env python3
import weakref
from time import sleep
class C:
def __init__(self, n):
self.n = n
weakref.finalize(self, self.finalizer, self.n)
@classmethod
def finalizer(cls, val: int):
print(f"In finalizer!")
print(f"Final message: {str(val)}")
a = C(32)
b = C(17)
# Create a weak reference to the object
weak_ref_a = weakref.ref(a)
weak_ref_b = weakref.ref(b)
# Check if the object is still referenced
if weak_ref_a():
print("Object 'a' is still alive.")
else:
print("Object 'a' has been garbage collected.")
if weak_ref_b():
print("Object 'b' is still alive.")
else:
print("Object 'b' has been garbage collected.")
print("registered")
del a
del b
print('deleted both objects')
print('calling garbage collection:')
import gc
gc.collect()
# Check if the object is still referenced
if weak_ref_a():
print("Object 'a' is still alive.")
else:
print("Object 'a' has been garbage collected.")
if weak_ref_b():
print("Object 'b' is still alive.")
else:
print("Object 'b' has been garbage collected.")
print("leaving in a moment ...")
sleep(1)There was a problem hiding this comment.
I approved the PR as I wouldn't write anything if it was classmethod originally.
But as we already have discussion I would like to add one consideration: I suggest changing self.finalizer to ConfigSession.finalizer or to type(self).finalizer if you think we need to support finalizer overriding.
This way anyone who reads code won't have to recheck that it is classmethod, and if someone doesn't think about this and removes @classmethod or overrides with object method they will get an error.
Yet again updated demo:
#!/usr/bin/env python3
import weakref
from time import sleep
class C:
def __init__(self, n):
self.n = n
weakref.finalize(self, self.finalizer, self.n)
@classmethod
def finalizer(cls, val: int):
print(f"In finalizer!")
print(f"Final meesage: {str(val)}")
class D(C):
@classmethod
def finalizer(cls, val: int):
print(f"D.finalizer with {val=}")
class OhNo(C):
def finalizer(cls, val: int):
print(f"OhNo.finalizer with {val=}")
a = C(32)
b = D(17)
c = OhNo(131)
print("registered")
del a
del b
del c
print("del all")
sleep(1)c is destructed only at exit.
If we add type(), we get an error in stdout (this is the way weakref.finalize work):
Exception ignored in: <finalize object at 0x7ffa4f148ae0; dead>
Traceback (most recent call last):
File "/nix/store/bbyp6vkdszn6a14gqnfx8l5j3mhfcnfs-python3-3.12.11/lib/python3.12/weakref.py", line 590, in __call__
return info.func(*info.args, **(info.kwargs or {}))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: OhNo.finalizer() missing 1 required positional argument: 'val'
There was a problem hiding this comment.
That's a good point. I'll adopt (some version of) those checks going forward. Thanks, @hedrok
| self, token: str = None, pid: int = None, on_error: Type[Exception] = None | ||
| self, | ||
| pid: int = None, | ||
| token: str = None, |
There was a problem hiding this comment.
Order of pid and token is changed, is this intentional?..
There was a problem hiding this comment.
Yes, that is the content of:
commit 3022334f79b5e34c26bac7ce1c6e716663061293
Author: John Estabrook <jestabro@vyos.io>
Date: Wed Oct 22 13:45:10 2025 -0500
T7910: switch keyword order pid/token for consistency and intuition
pid is the more commonly passed argument, and this maintains consistency
with configsession arg use.
|
|
||
| lst = json.loads(out.output) | ||
| if len(lst) == 1: | ||
| return lst[0] |
There was a problem hiding this comment.
Maybe it would be better to return list always?.. I think it can be quite unexpected, and it will be hard to use in scripts if there will be such need.
There was a problem hiding this comment.
Yes, it's odd, but for certain functions designed for internal use, it can make some sense; cf.
https://github.com/vyos/vyos-1x/blob/current/python/vyos/utils/config.py#L27-L45
d88996b to
4de1b6a
Compare
The standalone script teardown-config-session.py is called on CLI config-mode exit, to close the persistent vyconf config session. Instead of injecting the config-mode env var into the external script to indicate a non-ephemeral session, add keyword 'extant' to find existing session.
pid is the more commonly passed argument, and this maintains consistency with configsession arg use.
show_sessions returns a list of dicts of the internal session record structure for each live session. As this call is itself mediated by a session one can specify exclude_self=True, resp., exclude_other=True.
4de1b6a to
a02b7d5
Compare
|
CI integration ❌ failed! Details
|
|
Smoketest errors above unrelated to PR. |
vyosbot
added
mirror-completed
and removed
mirror-initiated
Change summary
Standardize VyconfSession instance lifetime management across its use in the various config modules.
In summary:
show_sessionsto return list of dicts of internal session record data structure, for data tracking and proof of correctnessTypes of changes
Related Task(s)
Related PR(s)
vyos/vyconf#32
How to test / Smoketest result
A screenshot showing the persistence and lifetime of a config-mode session:
Checklist: