Re: [PATCH v1 2/2] dts: improve starting and stopping interactive shells

[Jeremy Spewock <jspewock@iol.unh.edu>]

On Thu, Jul 11, 2024 at 10:53 AM Juraj Linkeš
<juraj.linkes@pantheon.tech> wrote:
>
> With the docs changes below,
> Reviewed-by: Juraj Linkeš <juraj.linkes@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()
>