Add support for shell v2 commands

[li-wjohnson]

Supports separate stdout + stderr as well as exit code.
Using adb source for implementation details (see https://android.googlesource.com/platform/system/core/+/master/adb/shell_protocol.h and related files).
Added tests and confirmed working with emulator.

I'm using Java's Process class and emulating parts of ProcessBuilder. Unfortunately, in order to handle both stdout and stderr, you need concurrency. To do this, I'm using an ExecutorService to spin up a thread which handles the stream demuxing.

[vidstige]

Thanks for this contribution. The code quality is very high. 🙇‍♂️ Some thoughts.

• Can this addition be made in such a way that no threading is needed? If a client needs both stderr and stdout they can perhaps use ExecutorService themselves? 🤔
• There is already a ITransportFactory that returns a Transport. I forgot all the details of this code, but it seems at least on the surface you can use this instead of creating an anonymous class in JadbDevice and just pass this and so on to keep things simple.

[vidstige]

Also what does the V2 refer to here? Can it be dropped?

[li-wjohnson]

Can this addition be made in such a way that no threading is needed? If a client needs both stderr and stdout they can perhaps use ExecutorService themselves?

Probably, but (1) it makes the code more complicated, as PipedOutputStream could no longer be used -- I'd have to write a custom OutputStream that blocks if we get the wrong output from the multiplexed stream and waits for another thread to read it -- and (2) I think it's a little more prone to user error -- users would always have to use a thread to read output streams, or else the code would deadlock.

There is already a ITransportFactory that returns a Transport. I forgot all the details of this code, but it seems at least on the surface you can use this instead of creating an anonymous class in JadbDevice and just pass this and so on to keep things simple.

Good point, let me change it.

Also what does the V2 refer to here? Can it be dropped?

Hmm, not sure where I came up with the "V2" nomenclature. ADB source seems to call it "ShellProtocol". I can reword things.

[li-wjohnson]

Oh, "v2" comes from the actual adb service command: shell,v2,raw:. Still I think ShellProtocol sounds a little better.

[vidstige]

Latest commit looks good. Still a bit worried about the threads. Perhaps it's the best solution, but could you explain a bit more. For example we already have a custom OutputStream to de-mangle blobs on the fly (here), allowing non-threaded IO. Perhaps something similar could be done? My assumption is that the common use case - To just forward stdout to stdout and stderr to stderr should not typically block?

Also a test seems to fail somehow.. 🤔 Otherwise looks great. Love the idea of reusing the standard library classes.

[vidstige]

✅ 🙇

[vidstige]

Sorry for late feedback, btw. Was on vacation. :)

[li-wjohnson]

To explain the thread problem in detail, the root of the issue is that the user can call any of 3 methods -- stdout read, stderr read, or waitFor (exitCode) -- but there's only one transport input stream to read from.

With the thread solution, our thread reads a chunk from the transport stream then (with the help of PipedInput/OutputStream) simply blocks until the user calls the right method - stdout read or stderr read. Since exitCode is one byte, we just store it.

Without the thread, each of stdout read, stderr read, and waitFor need to be able to do the above. This requires a pretty complicated system of synchronization/wait/notify - if stdout read is called but the underlying transport read returns a stderr chunk, it has to wait() until another thread calls stderr read to clear the stderr chunk and then notify()s the stdout thread that it should try to read again. If we want to handle the ProcessBuilder redirects, it adds an extra layer of complexity to deal with.

All of that is complicated, but doable. The worst part is the Process interface. Without a thread, it becomes impossible to try to run a shell command with a timeout. The normal pattern is using exitValue() in a loop. However, our exitValue() can never return true without the risk of blocking indefinitely -- the only way to know if the shell process has completed is by calling the transport's read(), which, in the absence of any stdout and stderr, will block until the shell process actually exits. With a thread, this becomes trivial, since we can just use the thread's future.

[vidstige]

Sweet. Last call - Are you sure about this threading thing? I would like if possible to leave out all the executor bits, and let callers do any threading as needed. It should be doable without blocking, even if stdout and stderr can be interleaved in the protocol. Anyway, this is a great contribution, and will merge soon. Just giving you some time to ponder first.

[vidstige]

To explain a bit more what I mean. Instead of reading from the stream and piping to stdout/stderr, we only read from the stream when someone else reads from e.g. the remote process stdin. If we get interleaved stderr packages, we just buffer then until someone reads them. This way the caller can use threads if they want, or just spawn a process and read stdin. 🤷‍♂ I don't know exactly what this would entail in terms of implementation, certainly a bit more hairy than what is in this PR, but the upside is a better library (imho)

[electromatter]

The main problem with buffering is that that buffer needs to be unbounded to prevent blocking of the remote process which can cause issues if the output is not bounded. Java doesn't have a nice way of select() on streams. You could avoid the use of a thread by breaking the Process abstraction and just provide an interface the streams the {STDOUT,STDERR,Exit} messages.

[vidstige]

Please rebase your branch on top of latest master to trigger a build 🙏