Fix pulling images with stream=True

[adw1n]

Pulling an image with option stream=True like this:

client.api.pull('docker.io/user/repo_name', tag='latest', stream=True)

without consuming the generator oftentimes results in premature drop of the connection. Docker daemon tries to send progress of pulling the image to the client, but it encounters an error (broken pipe) and therefore cancells the pull action:

Thread 1 "dockerd-dev" received signal SIGPIPE, Broken pipe.
ERRO[2018-09-03T05:12:35.746497638+02:00] Not continuing with pull after error: context canceled

As described in issue #2116, even though client receives response with status code 200, image is not pulled.

Closes #2116

Signed-off-by: Przemysław Adamek adw1n@users.noreply.github.com

[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 "i2116" git@github.com:adw1n/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.

[adw1n]

I doubt that that the pull log can be big enough to cause memory issues.

[shin-]

Hi @adw1n , thanks for the submission.

I'm not sure that's the right approach. Forcing stream=False seems hamfisted. If the problem is the one you describe, maybe we simply need a warning in the DockerClient documentation that when setting stream=True, the generator must be consumed for the operation to complete?

[adw1n]

@shin-

Forcing stream=False seems hamfisted.

I only suggested forcing it in docker.models.images.pull, because I don't see any benefits to using stream=True there. The only benefit to using stream=True is if you fear that the pull log can be very big (like >1MB) and your app will run out of memory.I doubt that can happen.

I do not force it in docker.api.image.pull

maybe we simply need a warning in the DockerClient documentation

I agree about adding the documentation (I've done it in docker.api.image.pull), although code in docker.models.images.pull definitelly needs to be changed, otherwise self.client.api.pull(repository, tag=tag, stream=True) will be called by the library code without consuming the generator.

My suggestions for docker.models.images.pull:

1. force stream=False (kwargs.pop('stream', None) or kwargs['stream']=False) + optionaly log a warning that stream=False doesn't make sense here
2. consume the generator if stream=True (best choice if the pull log can be big):

++        if kwargs.get('stream'):
++            for _ in self.client.api.pull(repository, tag=tag, **kwargs): pass
++        else:
++            self.client.api.pull(repository, tag=tag, **kwargs)
--        self.client.api.pull(repository, tag=tag, **kwargs)

or test if result of pull is a generator and consume it depending on that
3.

++ list(self.client.api.pull(repository, tag=tag, **kwargs))
-- self.client.api.pull(repository, tag=tag, **kwargs)

[adw1n]

@shin- ping

[shin-]

Sorry this took longer than it should have! I've made a few updates and merged the code in #2186 .

Thank you for your help and your patience!