On Thu, Jul 11, 2024 at 10:53 AM Juraj Linkeš <juraj.lin...@pantheon.tech> wrote: > > With the docs changes below, > Reviewed-by: Juraj Linkeš <juraj.lin...@pantheon.tech> > > > diff --git a/dts/framework/remote_session/interactive_shell.py > > b/dts/framework/remote_session/interactive_shell.py > > index 11dc8a0643..fcaf1f6a5f 100644 > > --- a/dts/framework/remote_session/interactive_shell.py > > +++ b/dts/framework/remote_session/interactive_shell.py > > @@ -9,6 +9,9 @@ > > collection. > > """ > > > > +import weakref > > +from typing import ClassVar > > + > > from .single_active_interactive_shell import SingleActiveInteractiveShell > > > > > > @@ -16,18 +19,26 @@ class InteractiveShell(SingleActiveInteractiveShell): > > """Adds manual start and stop functionality to interactive shells. > > > > Like its super-class, this class should not be instantiated directly > > and should instead be > > - extended. This class also provides an option for automated cleanup of > > the application through > > - the garbage collector. > > + extended. This class also provides an option for automated cleanup of > > the application using a > > + weakref and a finalize class. This finalize class allows for cleanup > > of the class at the time > > + of garbage collection and also ensures that cleanup only happens once. > > This way if a user > > + initiates the closing of the shell manually it is not repeated at the > > time of garbage > > + collection. > > """ > > > > + _finalizer: weakref.finalize > > + #: Shells that do not require only one instance to be running > > shouldn't need more than 1 > > + #: attempt to start. > > This wording is a bit confusing. This could mean that the shells could > require multiple instances. We could try an alternative approach: > One attempt should be enough for shells which don't have to worry about > other instances closing before starting a new one. > > Or something similar.
Good point, I'll make this change. > > > + _init_attempts: ClassVar[int] = 1 > > + > > def start_application(self) -> None: > > - """Start the application.""" > > + """Start the application. > > + > > + After the application has started, add a weakref finalize class to > > manage cleanup. > > I think this could be improved to: > use :class:`weakref.finalize` to manage cleanup Good idea, referencing the actual class like this is more clear. > > > + """ > > self._start_application() > > + self._finalizer = weakref.finalize(self, self._close) > > > > def close(self) -> None: > > - """Properly free all resources.""" > > - self._close() > > - > > - def __del__(self) -> None: > > - """Make sure the session is properly closed before deleting the > > object.""" > > - self.close() > > + """Free all resources using finalize class.""" > > Also here: > using :class:`weakref.finalize` Ack. > > > + self._finalizer() >
