Solves the https write to client error and add bytes counter

[gallir]

This patch solves the bug related to the "cannot write" error messages reported by several people. The problemd resided on the server connection not being closed when the client close its connection. I added a pipeAndClose() function that call to copyAndClose and is notified by channels. When one connection is closed the other is forced to close inmeditately.

Furthermore I added a couple of counters to ctx, it will serve to call a "onClosed" handler that can use this information (the next pull request).

[elazarl]

Hi,

Thanks for the PR.

I've had a fix for this problem, and it caused troubles, so I'm trying to understand the error better.

In which scenario the server won't be closed when the client closed the connection?

In the current implementation, after the client would close the connection, goproxy would also close its connection to the server.

[gallir]

You can repeat it, for example, with

wget -r -l 1 https://somedomain

Wget will reuse the existing connection until the last one, it then closes it but the upstream server never get notified because copyAndClose()'s that read from the proxyClient doesn't notify to the other that the client has closed the connection.

The connection to the server will be kept open until the server closes it by timeout (usually 60 seconds), then its corresponding writeAndClose() will try to write to the proxyclient... and this is the error message some people already noticed.

My patch uses two channels (in pipeAndClose) to synchronize between both writeAndClose(), when one of the finishes it forces the other one to close too with theOther.SetReadDeadline(time.Now()).

[elazarl]

but why don't copyAndClose work?

The reader goroutine should notice wget closed the connection to it,
doesn't it?

Then it would also close its connection to somedomain.

I still don't understand why this isn't working as I supposed it would.

Thanks

On Sun, Oct 18, 2015 at 2:59 AM, Ricardo Galli notifications@github.com
wrote:

You can repeat it, for example, with

wget -r -l 1 https://somedomain

Wget will reuse the existing connection until the last one, it then closes
it but the upstream server never get notified because copyAndClose()'s that
read from the proxyClient doesn't notify to the other that the client has
closed the connection.

The connection to the server will be kept open until the server closes it
by timeout (usually 60 seconds), then its corresponding writeAndClose()
will try to write to the proxyclient... and this is the error message some
people already noticed.

My patch uses two channels (in pipeAndClose) to synchronize between both
writeAndClose(), when one of the finishes it forces the other one to close
too with theOther.SetReadDeadline(time.Now()).

—
Reply to this email directly or view it on GitHub
#129 (comment).

[gallir]

1. Client connects to proxy, this connection is the "request"
2. handleHttps dials to the requested server, this connection is the "response".
3. Goproxy now has two differents TCP connections,
4. Two copyAndClose() are started, let call them "request2response" and "response2request"
5. Client requests a page from request, request2response copies it to response.
6. The web server answers, response2request copies to response.
7. Client closes the connection, request is closed and request2response finishes.
8. response2request is waiting on response and didn't notice request is already closed.
9. After 60 seconds the web server timeouts and closes response after sending some (timeout) message.
10. response2request tries to copy those last bytes to request, because is already closed it gives the warning.

It's not that I could happens, it always happens with the wget example I showed before. And the warning message is not the real problem, it's that goproxy keeps open and unused and useless connection until the server closes by timeout.

The right solution has to close the other connection when one is closed, by the server or the client (usually the latter).

[chripede]

This fix is really needed. HTTPS connections from the proxy to the target are never closed (until they time out). This fixes that.

Thank you!

[adamclerk]

I'm getting a similar error

Cannot write TLS response chunked trailer from mitm'd client: write tcp 172.18.0.2:8083->172.18.0.3:52144: write: broken pipe

Is this error part of the original class of error this PR was intended to fix? @gallir?

[BenKnigge]

Any chance that this is going to be merged soon?

[vbisbest]

@adamclerk I am getting that same error. Did you figure it out? I cant see that this was merged to see if it fixes it.

[adammartinai]

@vbisbest This was 4 years ago. I don't even remember this package.