Closed alexzeitgeist closed 7 years ago
If there is an error handled in the subsequent nil block, the connection is likely not reusable anyway (eg. "use of closed"). Further, a malicious server could result in that io.copy reading data forever (and thus consuming resources forever). If the client aborts, then the later io.copy aborts as well.
As such, I would rather just have a body close there and purposefully not reuse the connection for edge cases. If you change the code to just the defer close wrapped in a resp nil check, I would happily accept the pull req.
Looking at the http transfer code (go1.7) it looks like on close the body is read out up until the content length or maxPostHandlerReadBytes already, and gives up appropriately if the server does something fishy.
Ahhh, very interesting. It seems to avoid the issue when dealing with an malicious connection in the first link you referred to they only discard up to 512 bytes to have the connection reused. The code is still in there in the latest commit, and it seems, although it was pointed out that the net/http implementation of close() should take care of the body discard, it doesn't work as expected?
But I am not hard sold on this. I agree that it ought to be done in net/http directly. If you still like I can change the code to just include the defer close wrapped in the resp nil check.
Ok, after rereading the bug submit and bradfitz' explanation about a race condition that was caused by a flush, I fully agree that we should leave the response body draining code out. I updated the pull request accordingly.
thanks for the fix!
The http client should always close the response body - even when there is an error. See this link:
In addition, in order to be able to reuse the connection (if keep alive is enabled on the backend), the client should read and discard the remaining response data.