Fix handling output from tty-enabled containers. #669
Conversation
Treat output from TTY-enabled containers as raw streams, rather than as multiplexed streams. The docker API docs specify that tty-enabled containers don't multiplex. Also update tests to pass with these changes, and changed the code used to read raw streams to not read line-by-line, and to not skip empty lines. Addresses issue docker#630 Signed-off-by: Dan O'Reilly <oreilldf@gmail.com>
|
LGTM For future reference, you can always |
There was a problem hiding this comment.
I'm a bit miffed by that. It doesn't seem right that we would have to make additional API calls when streaming content. I understand why it's done obviously, but I'd like us to try to see if there's an alternative that doesn't incure additional calls.
There was a problem hiding this comment.
@shin- I share your concern - I just don't know what other option there is to determine if you're dealing with a TTY-enabled container or not. FWIW, it looks like the Docker CLI client uses the same technique (assuming I'm reading the source correctly).
There was a problem hiding this comment.
As an optimisation, attach (and _get_result) could take an optional argument to force TTY or non-TTY behaviour. Syntax to be determined, but e.g.:
client.attach(force_tty=True)
client.attach(force_no_tty=True)The inspect_container call could then be skipped.
This wouldn't need to be implemented as part of this PR - the pressing concern is to fix the bug.
There was a problem hiding this comment.
If that's how the docker client does it, I guess we'll have to do it as well. Let's merge for now - the fix is definitely needed. We can revisit later as @aanand said.
Fix handling output from tty-enabled containers.
As described in issue #630, getting the stdout/stderr from tty-enabled containers doesn't work properly, because docker-py treats the output as multiplexed, when docker is actually sending a raw stream. This patch attempts to address this by determining if a container to is tty-enabled before trying to read its output, and if it is, read the raw stream, rather than reading at a multiplexed stream.
I previously submitted a pull request for this issue here, but ended up screwing up the revision history in that branch, so I've opened a new one. Please see that request for comments on an earlier version of the diff.