Skip to content

Conversation

ibuildthecloud
Copy link
Contributor

Two changes are made in the PR.

  1. Switch to Requests.iter_lines() so that chunks are read properly. Docker 0.9.x introduced newlines into the streamed responses which surfaced a bug in docker-py in which it wasn't properly reading the HTTP chunked data.
  2. If the version of the API is >= 1.8 ensure that the stream=True parameter is sent to requests.

No tests are added in this PR because the existing integration tests were already failing against 0.9.0. This PR makes all integration tests succeed again.

Fixes #176

The previous code had a bug in which it assumed that the chunk data had
no newlines in it.  In 0.9.0 newlines were added to the stream results,
which exposed this bug.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 32 chunk_size is because request will block until the chunk size is read. The default is 512, so if your status is currently less than 512 length, you will get no content. If your reading the response to get progress updates, you want a smaller chunk_size. Setting to 1 would ensure that you get pretty close to real time status updates, but then you'll be looping a lot more.

@yukw777
Copy link
Contributor

yukw777 commented Mar 19, 2014

Nice! I'm going to close my pull request.

mpetazzoni added a commit that referenced this pull request Mar 19, 2014
@mpetazzoni mpetazzoni merged commit 7e105f5 into docker:master Mar 19, 2014
@mpetazzoni
Copy link
Contributor

Awesome, thanks!

@ibuildthecloud ibuildthecloud deleted the fix-chunking branch March 19, 2014 20:43
@joshuaconner
Copy link
Contributor

Is there any chance of doing a bugfix release for this? It would be awesome for docker-py to be fully functional under Docker 0.9.

@mpetazzoni
Copy link
Contributor

/cc @shin-

@ibuildthecloud
Copy link
Contributor Author

I'd also appreciate a release for this. I'm disabling certain functionality in my app because I can't use pull(stream=True).

@shin-
Copy link
Contributor

shin- commented Apr 4, 2014

0.3.1 should make it to pypi shortly. Sorry for the delay guys.

@mpetazzoni mpetazzoni added this to the 0.3.1 milestone Apr 8, 2014
aanand added a commit to aanand/docker-py that referenced this pull request Apr 23, 2014
Docker introduced newlines in stream output in version 0.9
(moby/moby#4276), but not to all
endpoints - POST /images/create, for example, does not include them.

This reverts to the old, less pleasant implementation of
_stream_helper(), with a manual check for newlines to fix the problem
described in docker#176 and fixed in docker#184, without the accompanying
regression. It should work against Docker 0.8, 0.9 and 0.10, both when
building and when pulling.
aanand added a commit to aanand/docker-py that referenced this pull request Apr 23, 2014
Docker introduced newlines in stream output in version 0.9
(moby/moby#4276), but not to all
endpoints - POST /images/create, for example, does not include them.

This reverts to the old, less pleasant implementation of
_stream_helper(), with a manual check for newlines to fix the problem
described in docker#176 and fixed in docker#184, without the accompanying
regression. It should work against Docker 0.8, 0.9 and 0.10, both when
building and when pulling.
aanand added a commit to aanand/docker-py that referenced this pull request Apr 28, 2014
Docker introduced newlines in stream output in version 0.9
(moby/moby#4276), but not to all
endpoints - POST /images/create, for example, does not include them.

This reverts to the old, less pleasant implementation of
_stream_helper(), with a manual check for newlines to fix the problem
described in docker#176 and fixed in docker#184, without the accompanying
regression. It should work against Docker 0.8, 0.9 and 0.10, both when
building and when pulling.
@mpetazzoni mpetazzoni modified the milestones: 0.3.2, 0.3.1 Apr 29, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

streaming from build() is broken in 0.3.0

5 participants