android: Fix adb background command PID detection

[marcbonnici]

Sometimes the reported PID for a background command would be reported
incorrectly. This was due to an assumption that the lowest PID
was always the command we were looking for, however this is not always
true and PID's could be recycled resulting in the PID of the search
command being reported instead.

To avoid this, filter the reported processes on the the grep command
itself to eliminate matching processes in the system and improve
detection of the PID of the desired background process.

[marcbonnici]

Should we return None or something else here to indicate an invalid PID? Perhaps 0 to be type consistent?

[douglas-raillard-arm]

In my experience, returning None or a made up value rather than raising does a lot more harm than good, since anything that actually needs the PID will not work with the fake value.

I guess we can end up in this situation if the process does not use stdin/stout/stderr and just exits immediately. On the ssh paramiko connection, PID detection is done by adding a print $$ before exec-ing the command, which is maybe the way to go.

[marcbonnici]

In my experience, returning None or a made up value rather than raising does a lot more harm than good, since anything that actually needs the PID will not work with the fake value.

That's a fair point. My concern over explicitly raising an error is that a usecase I can see for backgrounding a command is that you just want it to execute and don't care if its a short or long running task. In the case that it is short running and we start raising errors this might break existing flows that don't care about the PID.

On the ssh paramiko connection, PID detection is done by adding a print $$ before exec-ing the command, which is maybe the way to go.

That seems like a good idea, I'll try switching to that mechanism so that we can try and avoid this issue.

[douglas-raillard-arm]

In the case that it is short running and we start raising errors this might break existing flows that don't care about the PID.

I guess exceptions would have to be handled inside devlib mostly, e.g. when getting the PID to kill the command (although I think we would not do anything if the command is already finished). In any case, having a reliable PID would be a better if possible at all. It might complicate things in case of stream redirections though.

EDIT: we collect the PID no matter what, so it would indeed create issues if we raised exceptions and would make short commands unusable (at least in the current code organization)

[marcbonnici]

Agreed I think getting the reliable PID detection is the best way forward here. I've updated the PR with an attempt to align with the mechanism the SSH connection uses. From what I can tell the stream redirection was actually already taking place in this connection as well however the redirect_streams function was missing the modification to the returned stream. Please let me know if these changes look reasonable to you.

[douglas-raillard-arm]

Was the problem really because of PID reuse or just that if the command terminates before the lookup of its PID, the current code would just get the PID of the grep command ?

In any case, that's a good catch, thanks for debugging that.

[marcbonnici]

Was the problem really because of PID reuse or just that if the command terminates before the lookup of its PID, the current code would just get the PID of the grep command?

Perhaps reuse might not apply in this case and there was some delay introduce in spawning the initial command instead, however the result was the reported PID for the background command was returning the PID of the command used to find the PID rather than the background progress itself e.g.

In [13]: bg = ta.background("sleep 10")
    ...: print('PID: {}.format(bg.pid))
    ...: print('PID Found ', str(bg.pid) in ta.execute("ps"))

23534 /system/bin/sh -c (/data/local/tmp/bin/busybox ps -A -o pid,args | grep BACKGROUND_COMMAND_UUID=704c9a73dabc46e2af5da97d0abe6b61); echo "\n$?"
23536 /system/bin/sh -c BACKGROUND_COMMAND_UUID=704c9a73dabc46e2af5da97d0abe6b61 sh -c 'sh -c '"'"'export PATH=/data/local/tmp/bin:$PATH && export LC_ALL=C && sleep 1'"'"'  '
23539 grep BACKGROUND_COMMAND_UUID=704c9a73dabc46e2af5da97d0abe6b61

PID: 23534  # Should be: 23536 
PID Found: False  

This explains the "Process Not found" errors when trying to kill the processes as the grep had already completed, along with the leftover sleep processes preventing progress.

[douglas-raillard-arm]

That's minor but we could use echo $$; exec sh -c to avoid a layer

[douglas-raillard-arm]

We might need to double check the use of redirect_stream in ssh.py as well. Once the pipe is setup, it has to be consumed and closed properly. For Popen uses, it should be ok as Popen's context manager closes the streams on __exit__().

[douglas-raillard-arm]

Also we might want to do that only for stdout, since stderr should still be DEVNULL as we don't print anything to it.

[douglas-raillard-arm]

echo is unreliable on some shells, should be printf "%s\n" $$

[douglas-raillard-arm]

@marcbonnici I've updated my async PR with a patch that fixes that without needing to deal with complex redirections: b967194