[mozdevice.adb] Convert ADBProcess to act as a context manager

NEW
Unassigned

Status

Testing
Mozbase
4 years ago
4 years ago

People

(Reporter: jgraham, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

This prevents some code duplication and makes resource management easier.
Created attachment 8467765 [details] [diff] [review]
Make ADBProcess work as a ContextManager
Attachment #8467765 - Flags: review?(bclary)

Comment 2

4 years ago
Comment on attachment 8467765 [details] [diff] [review]
Make ADBProcess work as a ContextManager

Review of attachment 8467765 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the wait issue fixed.

I think the postprocess_func thing is neat, but if you don't mind, I'd like to reduce the generality by moving _get_exitcode to ADBProcess, changing ADBProcess to take a flag process_stdout[?] instead of a function and if the flag is set call self._get_exitcode() instead of self.postprocess_func(self). This will reduce or eliminate the chance of someone getting cute with ADBProcess and postprocess_func.

wlach, do you want to take a look?

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +29,5 @@
> +
> +
> +    The command is run on __enter__ with a maximum timeout set by the timeout
> +    attribute. If the timeout is reached the timedout attribute is set.
> +

This seems to imply that ADBProcess could be used by the user to execute processes. It was really only ever intended to serve to encapsulate some of the adb process information. I would like to make sure no one ever tries to use it directly. How about:

    ADBProcess is a context manager used to manage the process used to execute adb.

    It is designed to be used as the return value of the ``command`` and ``shell``
    methods and should never be used directly in other circumstances. This return
    value should always be used as the item of a with statement in order for the
    context manager methods __enter__ and __exit__ to be automatically called.

@@ +33,5 @@
> +
> +    :param args: A list of command arguments to run
> +    :param timeout: Maximum time to run the adb process, in seconds
> +    :param poll_interval: Interval at which to poll the adb process for completion
> +                          in seconds.

Let's use polling_interval to be consistent with ADBCommand and not use a default in the __init__ since we don't use a default timeout.

@@ +35,5 @@
> +    :param timeout: Maximum time to run the adb process, in seconds
> +    :param poll_interval: Interval at which to poll the adb process for completion
> +                          in seconds.
> +    :param postprocess_func: Optional function taking this object as the argument to
> +                             run after the process has successfully completed

This is a neat way to call _get_exitcode but I'm a little concerned in the generality. I'm almost inclined to move _get_exitcode to ADBProcess and use a flag in the constructor instead of a function to tell __enter__ to post process the stdout to obtain the real exit code. What do you think?

@@ +60,2 @@
>      """
> +    def __init__(self, args, timeout, poll_interval=0.1, postprocess_func=None):

def __init__(self, args, timeout, polling_interval, postprocess_func=None):

@@ +62,3 @@
>          self.args = args
> +        self.timeout = timeout
> +        self.poll_interval = poll_interval

self.polling_interval = polling_interval

@@ +121,5 @@
> +        while (time.time() - start_time) <= self.timeout:
> +            exit_code = self.proc.poll()
> +            if exit_code is not None:
> +                self.timedout = False
> +                break

It is possible though rare that the process could terminate after this if block and before the beginning of the loop and that the elapsed time might exceed the timeout causing a timeout error even though the process completed normally. I think the previous code is clearer and more correct.

@@ +122,5 @@
> +            exit_code = self.proc.poll()
> +            if exit_code is not None:
> +                self.timedout = False
> +                break
> +            time.sleep(self.poll_interval)

time.sleep(self.polling_interval)

@@ +272,4 @@
>          if timeout is None:
>              timeout = self._timeout
>  
> +        return ADBProcess(args, timeout, poll_interval=self._polling_interval)

return ADBProcess(args, timeout, self._polling_interval)

@@ +831,5 @@
>          if timeout is None:
>              timeout = self._timeout
>  
> +        return ADBProcess(args, timeout, poll_interval=self._polling_interval,
> +                          postprocess_func=self._get_exitcode)

return ADBProcess(args, timeout, self._polling_interval, postprocess_func=self._get_exitcode)
Attachment #8467765 - Flags: review?(wlachance)
Attachment #8467765 - Flags: review?(bclary)
Attachment #8467765 - Flags: review+
(In reply to Bob Clary [:bc:] from comment #2)
> @@ +35,5 @@
> > +    :param timeout: Maximum time to run the adb process, in seconds
> > +    :param poll_interval: Interval at which to poll the adb process for completion
> > +                          in seconds.
> > +    :param postprocess_func: Optional function taking this object as the argument to
> > +                             run after the process has successfully completed
> 
> This is a neat way to call _get_exitcode but I'm a little concerned in the
> generality. I'm almost inclined to move _get_exitcode to ADBProcess and use
> a flag in the constructor instead of a function to tell __enter__ to post
> process the stdout to obtain the real exit code. What do you think?

I'm a little concerned that this leads to tight coupling between the ADBDevice class and the ADBProcess class; the ADBProcess suddenly has to know about the exact format used by ADBDevice to store the exit code in the output.
Comment on attachment 8467765 [details] [diff] [review]
Make ADBProcess work as a ContextManager

Review of attachment 8467765 [details] [diff] [review]:
-----------------------------------------------------------------

I think I agree with jgraham on the exit code / post process stuff. Other than that, I think I pretty much agree with all of bc's comments.
Attachment #8467765 - Flags: review?(wlachance) → review+
Created attachment 8469415 [details] [diff] [review]
Make ADBProcess work as a ContextManager - v2

I think I updated all the necessary things; please verify.
Attachment #8469415 - Flags: review?(bclary)

Comment 6

4 years ago
(In reply to William Lachance (:wlach) from comment #4)
> Comment on attachment 8467765 [details] [diff] [review]
> Make ADBProcess work as a ContextManager
> 
> Review of attachment 8467765 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think I agree with jgraham on the exit code / post process stuff. Other
> than that, I think I pretty much agree with all of bc's comments.

re _get_exit_code, Ok I'll defer to everyone's judgement but let's make sure during reviews to keep people from abusing ADBProcess. For the most part people should just be using {command,shell}_output and shell_bool and not mucking around in the lower level stuff.

(In reply to James Graham [:jgraham] from comment #5)
> Created attachment 8469415 [details] [diff] [review]
> Make ADBProcess work as a ContextManager
> 
> I think I updated all the necessary things; please verify.

Will do. Keeping up with you is a full time job! ;-)

Updated

4 years ago
Attachment #8469415 - Attachment description: Make ADBProcess work as a ContextManager → Make ADBProcess work as a ContextManager - v2

Comment 7

4 years ago
Comment on attachment 8469415 [details] [diff] [review]
Make ADBProcess work as a ContextManager - v2

Review of attachment 8469415 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the ADBProcess docstring change.

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +17,5 @@
> +    This object must be used as a context manager e.g.
> +
> +    with ADBProcess(["shell", "cat /etc/hosts"], timeout=30) as process:
> +        hosts_data = process.stdout_file.read()
> +

I really don't want this example here. I think it will give the wrong idea to people about how to use ADBProcess. See the original review comment.
Attachment #8469415 - Flags: review?(bclary) → review+
Created attachment 8469911 [details] [diff] [review]
Make ADBProcess work as a ContextManager

I think I'd already made that change but uploaded an old version of the patch or something.
Attachment #8467765 - Attachment is obsolete: true
Attachment #8469415 - Attachment is obsolete: true

Comment 9

4 years ago
jgraham, I am having problems with this patch. I see a much increased number of failures when detecting if the device is ready in adb_android.py:is_device_ready for my gs3s in particular. The problem occurs in the loop for performing the pm command.

I've tried and tried to figure it out but have failed so far. It is to the point where it is affecting my ability to do the other work I am supposed to be doing. 

Since this bug is not about fixing an error but is more about changing the code to fit with specific python idioms, I don't see it as being so important at this moment.

Let's hold off on this bug/patch for the moment and get your other bugs landed without it. I promise I will continue to work on figuring out the problem I am having in my off time.
OK, no problem.
You need to log in before you can comment on or make changes to this bug.