handle partial writes correctly

[zwily]

This is an attempt to fix #26. os.write() does not guarantee that it will write the entire string given. This change creates a buffer to store the remainder of partial writes, and adds streams with pending data to the main select loop so we can attempt to flush the data.

[zwily]

I don't have much Python experience and wasn't quite sure how to go about testing this.

[codekitchen]

for what it's worth, I pulled this into my local docker-compose install and it has fixed all issues with missing output in my testing :+1:

[aanand]

This fixes truncation for me, but re-introduces the hanging that was introduced by https://github.com/d11wtq/dockerpty/commit/6ded9260d9c7f9d4761fbc2d70281b994d159126 and removed in #22.

[zwily]

How do you replicate that?

[zwily]

@aanand I can't replicate the hang. If I start a container with bash and let it idle for a few minutes, I can keep using it after that.

I think the hang introduced in 6ded926 is because it was attempting to flush() every pump, even ones not ready to read. That could cause a read on an fd not ready for reading to hang. My change doesn't attempt to flush every pump on every loop, so that same hang shouldn't happen.

How did you do it?

[zwily]

OK, I think I see what it is... When using dockerpty directly I can't get it to hang, but when fig, it will hang on any interactive stream. Digging in.

[aanand]

Here's what I do to reproduce:

1. Check out your branch
2. Make a fresh virtualenv
3. Run python setup.py develop
4. Run pip install docker-compose
5. Run this script:

#!/usr/bin/env python

from compose.cli.docker_client import docker_client
import dockerpty

client = docker_client()
container = client.create_container(image="debian:wheezy", stdin_open=True, tty=True, command='seq 2000')
dockerpty.start(client, container, interactive=True)

I see all 2000 lines printed out, but afterwards it hangs.

If I then check out master and run the same script, it never hangs, but I sometimes get incomplete output.

[aanand]

I've also observed that it's easier to reproduce using Compose than with straight dockerpty. I wonder if it's to do with timing - with Compose it's possible there's a bit of a delay between creating and starting the container, for example.

[zwily]

@aanand - Try again with the new addition. Now it makes sure not to wait for stdin on your pty to close before considering the pumps done.

[aanand]

OK, this now seems to work. Thanks! Not sure why the tests are failing though.

Can any of it be simplified? If _hijack_tty is poking aroud inside Pump's attributes, that suggests Pump isn't a great abstraction right now.

[zwily]

I've dug into the test failures a bit, and I'm perplexed. It looks like with my patch, some streams are getting crossed or something. I'm trying to dig more today.

As for simplification... probably. This was a quick-and-dirty hack to get things working.

[zwily]

Finally got all tests passing.

[aanand]

OK, I'm satisfied that I understand this change.

Could any of the new methods (e.g. needs_write and close) be more fully unit tested?

[zwily]

@aanand I added a few more tests.

[aanand]

Nice. Could you squash to one commit please? Then I think we're ready to go.

ping @bfirsh @dnephin

[zwily]

@aanand Done.

[aanand]

LGTM

[dnephin]

LGTM! Thanks for fixing this!

[delfick]

One problem with this code that I just realised is that because we actually close the streams, if you're using sys.stdout and want to do anything after using dockerpty, it fails.

Do we have to actually close the streams themselves? or can we just mark them as closed as far as dockerpty is concerned?

[zwily]

@delfick At least in one case, you definitely want to close the stream. That's when you're doing something like cat file | docker-compose run --rm -T web cat. If you don't close stdout, the cat process on the other side will never get an EOF, and so will never exit. Maybe it depends on if the session is interactive or not?

[delfick]

In that case, perhaps an option to say as the user of dockerpty I'll handle closing the streams?

On Sun, 10 May 2015 23:10 Zach Wily notifications@github.com wrote:

@delfick https://github.com/delfick At least in one case, you definitely want to close the stream. That's when you're doing something like cat file | docker-compose run --rm -T web cat. If you don't close stdout, the cat process on the other side will never get an EOF, and so will never exit. Maybe it depends on if the session is interactive or not?

— Reply to this email directly or view it on GitHub https://github.com/d11wtq/dockerpty/pull/36#issuecomment-100638649.

[delfick]

Though the streams being closed are not related to the streams connected to the web cat and so I don't think it would make a difference.

On Sun, 10 May 2015 23:13 Stephen Moore delfick755@gmail.com wrote:

In that case, perhaps an option to say as the user of dockerpty I'll handle closing the streams?

On Sun, 10 May 2015 23:10 Zach Wily notifications@github.com wrote:

@delfick https://github.com/delfick At least in one case, you definitely want to close the stream. That's when you're doing something like cat file | docker-compose run --rm -T web cat. If you don't close stdout, the cat process on the other side will never get an EOF, and so will never exit. Maybe it depends on if the session is interactive or not?

— Reply to this email directly or view it on GitHub https://github.com/d11wtq/dockerpty/pull/36#issuecomment-100638649.

[aanand]

If you don't close stdout, the cat process on the other side will never get an EOF, and so will never exit.

If I understand you, that means that in order for the container to exit, we need to close the TCP stream that's sending stdin to the container... but we don't need to close the local stdin/out/err. Is that right?

If so, I don't see any obvious harm in keeping them open and simply returning. In the case of Compose, the next thing it will do is exit, so it makes no difference to me, but if other people want to keep using stdin/out/err then it'll be a useful change.