implement stream demultiplexing for exec commands

[little-dude]

Hi there,

This is an attempt to fix #1952: offering the possibility to demultiplex stdout, stderr and stdin. Currently, it's only implemented for docker exec but can be extended to other APIs where it's relevant.

The implementation is not complete: some tests need to be added, and some needs to be adjusted. But since I'm new to this codebase, I wanted to open the PR to get early feedback, before spending time polishing it.

Here is an example of how it works:

# Create a container that runs in the background
>>> client = docker.from_env()
>>> container = client.containers.run(
...     'bfirsh/reticulate-splines', detach=True)

# Prepare the command we are going to use. It prints "hello stdout"
# in `stdout`, followed by "hello stderr" in `stderr`:
>>> cmd = '/bin/sh -c "echo hello stdout ; echo hello stderr >&2"'

# With stream=False and demux=False, the output is a string
# that contains both the stdout and the stderr output:
>>> res = container.exec_run(cmd, stream=False, demux=False)
>>> res.output
b'hello stderr\nhello stdout\n'

# With stream=True, and demux=False, the output is a
# generator that yields strings containing the output of both
# `stdout` and `stderr`:
>>> res = container.exec_run(cmd, stream=True, demux=False)
>>> next(res.output)
b'hello stdout\n'
>>> next(res.output)
b'hello stderr\n'
>>> next(res.output)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
StopIteration

# With stream=True and demux=True, the generator now
# separates the streams, and yield 3-tuples
# (stdin, stdout, stderr):
>>> res = container.exec_run(cmd, stream=True, demux=True)
>>> next(res.output)
(None, b'hello stdout\n', None)
>>> next(res.output)
(None, None, b'hello stderr\n')
>>> next(res.output)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
StopIteration

# Finally, with stream=False and demux=True, the whole output
# is returned, but the streams are still separated:
>>> res = container.exec_run(cmd, stream=True, demux=True)
>>> next(res.output)
(None, b'hello stdout\n', None)
>>> next(res.output)
(None, None, b'hello stderr\n')
>>> next(res.output)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
StopIteration

fixes #1952

[GordonTheTurtle]

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "master" git@github.com:little-dude/docker-py.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

[little-dude]

@shin- do you mind taking a quick look at this? I'll have some time this week to fix the tests (and add some for this feature). If you have any comment I'll handle them as well.

[little-dude]

Tests should pass now. I'd like to add a few other tests for demux=True though, because _read_from_socket is a bit more complex now. Fwiw I've been using that for the past two weeks and it works fine for me.

[little-dude]

@shin- any chance of getting this reviewed? It's quite demoralizing to not get any feedback...

[shin-]

Sorry - it looked like you still had test failures, but they're due to moby/moby#37920

Can you rebase against the current master to pick up a3111d9 and clear the CI failures?

[little-dude]

Thank you! Rebased. I'm not on my dev machine right now though, so I haven't re-run the tests locally. Let's see what CI has to say about this.

Edit: it looks ok :)

[little-dude]

@shin- CI passed. What do you think? As I said I can add additional tests, but I'd like to know you like the implementation first to avoid extra work.

[shin-]

@little-dude Thanks!

Design-wise, I don't think there's ever a case where the data we receive from the server is in the STDIN channel. If we could remove that and go from a 3-tuple to a 2-tuple on demux, I think it would make the experience better overall.

[shin-]

Good on design overall, just a few style / organizational nits

[shin-]

Not sure this is the best place for such a long example, especially since it applies to attach() as well.

[little-dude]

I'm not sure where to put it though. I can just copy paste if for attach(), or add a reference to it.

[shin-]

Maybe as a separate docs page. We can figure that out later though, it's not worth blocking the PR.

[little-dude]

Thanks a lot for the review! I'll address the comments and add a test or two.

Design-wise, I don't think there's ever a case where the data we receive from the server is in the STDIN channel.

Yeah I was confused by this as well, this is a weird API.

[little-dude]

Addressed the comment (except the one about the docstring), and rebased on top of #2178 for now to prevent some pytest failures.

I still need to add a test for the new API.

[shin-]

Looks like Jenkins caught a bit of a cold over the weekend! I kicked the build again, but everything looks solid now.

[shin-]

Found a couple outdated docstrings, otherwise LGTM

[little-dude]

Rebased, comments addressed! I added a bunch of unit tests (and found a bug!). I'll look at the integration tests to see if there's something I can easily add there.

[little-dude]

I found a couple places where I could add tests. I'll update the PR in a few hours.

[little-dude]

@shin- I think this is ready (provided that CI passes).

[shin-]

Thanks! I'll give it another round of review.

[little-dude]

Found a couple mentions to "stdin" I need to remove, and left a comment about an inconsistency (I think it's fixable, I'm just not sure if it's ok to do that in another PR later).

[shin-]

Merged - thank you for your help and your patience!

[little-dude]

Thanks a lot for your reviews @shin- !