[Hexagon] Less aggressive adb state clean up

[csullivan]

Changes:

• Issue SIGINT before SIGKILL to allow the server time to clean up and unbind the port on Android
• Only remove forward (and reversed) ports set during a given session rather than globally applying remove all.

[csullivan]

cc @mehrdadh @Lunderberg this one was an attempt to stabilize the adb issues we've seen. I'm not sure I see much improvement, but would like your feedback. Thanks

[Lunderberg]

LGTM. There's a couple of questions and musing on edge cases, but I think changes as-is are an improvement over the existing behavior, so we can have them in follow-up PRs if necessary.

[Lunderberg]

Do we want to clean up port forwards that have been opened for more than N days? The .stop_server() method may not be called if a segfault occurs, and that may leave ports open unnecessarily.

[Lunderberg]

If a port forward already exists, does adb exit with a non-zero return code? If the adb call exits with a zero return code in that case, then there are two processes trying to start port forwards on overlapping ranges would result in double clean-up occurring.

[Lunderberg]

Also, I think we should move the self.forwarded_ports_.append(port) to be after the subprocess call. That way, a cleanup that occurs as the stack unwinds (e.g. the hexagon_launcher fixture) wouldn't try to clean up the failed port forward.

[csullivan]

Thank you for the thought on this, and no adb doesn't complain about multiple attempts to forward the same port. I decided to be more rigorous with port checking and only forward ports that are not already forwarded by adb or bound by another process. This seems to have greatly improved the stability. PTAL at ed610c4.

[Lunderberg]

I wonder if we could set the process group for the subprocesses launched by PopenWorker. Those are the ones that tend to be left-over. The pkill -P command would send signals to the direct children of the main process, but not to any grandchildren (e.g. main process starts a worker subprocess, which starts a compilation subprocess), or to any processes started after the child process is killed but before the parent process is killed (e.g. main process interprets the closing child as a failed test case, starts a new test case). It looks like it would be relatively straightfoward to have Popen calls started as the same process group, so that they would all receive the signal simultaneously.

[Lunderberg]

LGTM, just a few general nitpicks that could be handled either here or in a follow-up.

[Lunderberg]

Nit: Rename to _existing_forwarded_ports or _get_forwarded_ports. The current name reads as if this is performing some processing on each forwarded port.

[Lunderberg]

I believe the total number of ports isn't important, instead trying to match the port range that may be attempted a server, when it searches for an available port to listen on. How often is the port already forwarded or in use, and would be be better to throw an error in those cases?

[csullivan]

Not sure throwing an error in these cases makes sense given that the user only cares about the functionality of their tests on hardware and doesn't want to see test failures based on bad port configurations.

If we change the RPC server to fail rather than search for a new port as it does now, then we can revisit the launcher code and have it try until successfully binding to a single port and remove the port range nonsense.

[Lunderberg]

Good point, and that would make a lot of sense to me. I like the semantics of "bind here or error" much more than "bind here or maybe somewhere else or error". That makes sense for it to be a later change, and not something needed at the moment.

[Lunderberg]

Nit: Since existing_forwards isn't used elsewhere, may be cleaner to generated it inside _forward_ports.

[mehrdadh]

LGTM, thanks for fixing this!