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

Juraj Linkeš juraj.linkes at pantheon.tech
Thu Jul 11 16:53:28 CEST 2024


With the docs changes below,
Reviewed-by: Juraj Linkeš <juraj.linkes at 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.

> +    _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

> +        """
>           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`

> +        self._finalizer()



More information about the dev mailing list