Capture emulator stdout during startup

[rmartin16]

Changes

• An attempt to resolve Android emulator does not open until CTRL+C is pressed during "Starting emulator..." #1573
• One user in Discord has reported that reading stdout resolves the issue
• Introduces PopenOutputStreamer for encapsulating the output stream Thread and adds the ability to capture instead of print output
• Adds Subprocess.stream_output_non_blocking() alongside the existing Subprocess.steam_output() to facilitate callers streaming (and conditionally capturing output) while doing other tasks

PR Checklist:

• All new features have been tested
• All new features have been documented
• I have read the CONTRIBUTING.md file
• I will abide by the code of conduct

[freakboy3742]

Broadly makes sense (I think...); a couple of nitpicks inline, but on the whole this looks like a good refactoring of a complex part of the streaming code.

The only other issue I noticed is a weird one - the code clearly has 100% coverage, but I can't see how the capture_output = False case is being exercised. I'm guessing it's being tested implicitly by another test somewhere, but given it's at the core of how this works, it would be worth having an explicit test.

To make sure I'm understanding what is going on here - the key to the problem is that the old Popen usage wasn't ever invoking readline(), which apparently causes Windows to lock up and not produce output. The code now always uses a background thread to poll and collect stdout (not just for the stream_output() case). The Android usage previously only used communicate() to gather output in the case of an error, and otherwise ignored output. The new implementation does continuous readline() polling to get output as it is generated, which should (hopefully 🤞) unblock Windows.

Have I got that right?

[freakboy3742]

I'd be inclined to wrap this as a streamer.stop() method (or similar), rather than reaching into the internal implementation so if we ever have more complex "kill the streamer" logic, we don't miss anything that is required.

[rmartin16]

That makes sense; although, I'm inclined to call it streamer.request_stop() because readline() is blocking and as such, we can't guarantee setting this flag will actually ever cause the thread to exit (....you know, if its stuck in readline()).

[freakboy3742]

Sure the exact name doesn't really concern me, it's more the conceptual wrapping of the behavior, rather than reaching into internals. Also stop() could easily be confused for a method on the core thread API (it doesn't AFAICT, but stop() would be an obvious conceptual match for run()... but it won't behave that way).

[rmartin16]

RE: stopping threads

I always loved this blog post from raymond chen....although, he seemingly removed it...

https://web.archive.org/web/20190108101649/https://blogs.msdn.microsoft.com/oldnewthing/20150814-00/?p=91811

[freakboy3742]

Missing a description here

[rmartin16]

The only other issue I noticed is a weird one - the code clearly has 100% coverage, but I can't see how the capture_output = False case is being exercised. I'm guessing it's being tested implicitly by another test somewhere, but given it's at the core of how this works, it would be worth having an explicit test.

Would you consider the test_output() test in tests/integrations/subprocess/test_PopenOutputStreamer.py to cover this case? Or maybe something more?

To make sure I'm understanding what is going on here - the key to the problem is that the old Popen usage wasn't ever invoking readline(), which apparently causes Windows to lock up and not produce output. The code now always uses a background thread to poll and collect stdout (not just for the stream_output() case). The Android usage previously only used communicate() to gather output in the case of an error, and otherwise ignored output. The new implementation does continuous readline() polling to get output as it is generated, which should (hopefully 🤞) unblock Windows.

Have I got that right?

Right....the theory is if we read stdout from the emulator, it'll stop blocking it from properly opening for users.

More broadly, this provides the option of creating a Popen and instead of only being able to call Subprocess.stream_output() to read stdout while blocking, there's now Subprocess.stream_output_non_blocking() to (obviously) not block while reading stdout.

[freakboy3742]

The only other issue I noticed is a weird one - the code clearly has 100% coverage, but I can't see how the capture_output = False case is being exercised. I'm guessing it's being tested implicitly by another test somewhere, but given it's at the core of how this works, it would be worth having an explicit test.

Would you consider the test_output() test in tests/integrations/subprocess/test_PopenOutputStreamer.py to cover this case? Or maybe something more?

Ah - that was the case I was missing. It's relying on the default value of the argument.

Have I got that right?

Right....the theory is if we read stdout from the emulator, it'll stop blocking it from properly opening for users.

👍

More broadly, this provides the option of creating a Popen and instead of only being able to call Subprocess.stream_output() to read stdout while blocking, there's now Subprocess.stream_output_non_blocking() to (obviously) not block while reading stdout.

Yeah - that's a nice API addition. Not sure if we'll have any use for it, but it's a nice club to have in the bag just in case, and the rest of the refactoring around adding that API are all nice improvements as well.

[freakboy3742]

All looks good... now to cross our fingers and hope it actually fixes the windows emulator lockup problem 🤞