Skip to content
This repository was archived by the owner on Jun 20, 2024. It is now read-only.

[proxy] rewrote chunked response handler #1112

Merged
merged 1 commit into from
Jul 20, 2015
Merged

Conversation

paulbellamy
Copy link
Contributor

  1. We cannot send "Connection: close", because the fsouza docker client
    expects the tcp socket to stay open between requests.

  2. Because we cannot force-close the connection, we can't hijack the
    connection (because go's net/http doesn't let use un-hijack it).

  3. Because we need to maintain the individual chunking of messages (for
    docker-py), we can't just copy the response body, as Go will remove and
    re-add the chunking willy-nilly.

Therefore, we have to read each chunk one-by-one, and flush the
ResponseWriter after each one.

Fixes #1103

Merging into master instead of 1.0, as it is definitely not a "patch" change. Replacing #1110.

@rade rade removed their assignment Jul 10, 2015
if err == nil {
err = chunks.Err()
}
if err != nil {

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@paulbellamy paulbellamy force-pushed the 1103-proxy-chunking branch 2 times, most recently from d04ca3f to 951bc23 Compare July 14, 2015 14:35
@squaremo squaremo self-assigned this Jul 20, 2015
@squaremo
Copy link
Contributor

To check my understanding of what's changed: we no longer hijack the downstream connection to the client, just the upstream connection to docker. Docker is fine with the connection being closed arbitrarily (or we are assuming that); some clients are not.

Is there a reason we have to parse the hijacked upstream, rather than using ChunkedReader as before?

  1. We cannot send "Connection: close", because the fsouza docker client
    expects the tcp socket to stay open between requests.

Just to be clear, this is a bug in that client, right? It should deal with the HTTP connection being closed arbitrarily.

@paulbellamy
Copy link
Contributor Author

we no longer hijack the downstream connection to the client, just the upstream connection to docker.

Yes. if we hijack a connection we are forced to close the connection, as there is no "unhijack".

Docker is fine with the connection being closed arbitrarily (or we are assuming that); some clients are not.

Yes

Is there a reason we have to parse the hijacked upstream, rather than using ChunkedReader as before?

ChunkedReader does not expose individual chunks. It just smashes them together into a bytestream. The downstream client (docker-py in particular) expects json objects and http chunks to align.

Just to be clear, this is a bug in that client, right? It should deal with the HTTP connection being closed arbitrarily.

Yes

@squaremo
Copy link
Contributor

Is there a reason we have to parse the hijacked upstream, rather than using ChunkedReader as before?

ChunkedReader does not expose individual chunks. It just smashes them together into a bytestream. The downstream client (docker-py in particular) expects json objects and http chunks to align.

Ahh I beg your pardon, I had it the wrong way around. The ChunkedReader was to gate the straight-through byte copy wasn't it.

@paulbellamy
Copy link
Contributor Author

net/http/httputil.ChunkedReader just smashes all the bytes together, yes. Our scanner lets us flush after each chunk.

@paulbellamy paulbellamy force-pushed the 1103-proxy-chunking branch from 951bc23 to 11663dd Compare July 20, 2015 13:42
1) We cannot send "Connection: close", because the fsouza docker client
   expects the tcp socket to stay open between requests.

2) Because we cannot force-close the connection, we can't hijack the
   connection (because go's net/http doesn't let use un-hijack it).

3) Because we need to maintain the individual chunking of messages (for
   docker-py), we can't just copy the response body, as Go will remove and
   re-add the chunking willy-nilly.

Therefore, we have to read each chunk one-by-one, and flush the
ResponseWriter after each one.
@squaremo squaremo merged commit 11663dd into master Jul 20, 2015
@paulbellamy paulbellamy deleted the 1103-proxy-chunking branch July 20, 2015 14:49
@rade rade modified the milestone: 1.1.0 Jul 21, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Weave proxy unexpectedly closes the connection in chunked responses
5 participants