Bug 169878 - webkitpy: Work around simctl launch returning dead processes
Summary: webkitpy: Work around simctl launch returning dead processes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-20 10:39 PDT by Jonathan Bedard
Modified: 2017-03-20 14:34 PDT (History)
3 users (show)

See Also:


Attachments
Patch (5.36 KB, patch)
2017-03-20 10:41 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (6.53 KB, patch)
2017-03-20 10:58 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (6.83 KB, patch)
2017-03-20 13:14 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2017-03-20 10:39:06 PDT
Sometimes, simctl launch will return the PID of a dead process.  Check that the process we return when launching an app is actually running.
Comment 1 Jonathan Bedard 2017-03-20 10:41:23 PDT
Created attachment 304932 [details]
Patch
Comment 2 Jonathan Bedard 2017-03-20 10:58:19 PDT
Created attachment 304935 [details]
Patch
Comment 3 Daniel Bates 2017-03-20 12:27:54 PDT
Comment on attachment 304935 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=304935&action=review

> Tools/ChangeLog:7
> +

Please explain the purpose of this change and the issue we are working around.

> Tools/ChangeLog:17
> +        (SimulatedDevice.launch_app): Check that the process being returned is active, use a timeout instead
> +        of attempts.

We tend to wrap ChangeLog lines at ~100 characters.

> Tools/Scripts/webkitpy/xcode/simulated_device.py:181
> +            if match and self.poll(int(match.group('pid'))) is None:

Please file a radar on CoreSimulator (if one does not already exist) to have "simctl launch" returns us a valid process id and add a comment to above this line that explains the reason we need to query the OS to see if the pid returned by "simctl launch_app" is valid as well as references the radar URL.

> Tools/Scripts/webkitpy/xcode/simulated_device.py:195
> +    def poll(self, pid):
> +        try:
> +            os.kill(pid, 0)
> +        except OSError as err:
> +            assert err.errno == errno.ESRCH
> +            return 1
> +        return None

I know that you are motivated to share this code with SimulatorProcess.Popen.poll(). This does not seem like the appropriate place for this function. I wish we could find a more appropriate place to put this.
Comment 4 Jonathan Bedard 2017-03-20 13:14:43 PDT
Created attachment 304944 [details]
Patch
Comment 5 Jonathan Bedard 2017-03-20 13:20:25 PDT
(In reply to comment #3)
> Comment on attachment 304935 [details]
> Patch
>
> > Tools/Scripts/webkitpy/xcode/simulated_device.py:195
> > +    def poll(self, pid):
> > +        try:
> > +            os.kill(pid, 0)
> > +        except OSError as err:
> > +            assert err.errno == errno.ESRCH
> > +            return 1
> > +        return None
> 
> I know that you are motivated to share this code with
> SimulatorProcess.Popen.poll(). This does not seem like the appropriate place
> for this function. I wish we could find a more appropriate place to put this.

I believe this is the appropriate place for this.  SimulatorProcess receives a device as an argument, it is this device which should define the behavior of poll.  The other option would be to define poll in the port class, but other Darwin ports either do not need a poll function or would define their respective polls differently.
Comment 6 WebKit Commit Bot 2017-03-20 14:34:01 PDT
Comment on attachment 304944 [details]
Patch

Clearing flags on attachment: 304944

Committed r214192: <http://trac.webkit.org/changeset/214192>
Comment 7 WebKit Commit Bot 2017-03-20 14:34:05 PDT
All reviewed patches have been landed.  Closing bug.