Allow connection close for custom streams #1603
Conversation
|
Interesting. Don't have access to a computer right now. Will have a deeper look next week. |
| if err != nil { | ||
| hc.releaseReader(br) |
There was a problem hiding this comment.
@erikdubbelboer this is another change that would be required to avoid data races if the connection can be closed from outside. The reader shall only be released directly if it is not a stream connection. Otherwise do it in the cleanup method called via the closer interface.
The reason is that if the reader is realeased earlier, it would then be available for the next request and might cause a data race.
|
@erikdubbelboer I did fix the |
|
Thanks! Some tests can be flaky, especially on windows. It all looks good now. |
We do use fasthttp for our project as a proxy between an uplink connection and a target. We discovered the following scenario where we run into an issue that we can't solve without a minor change to the fasthttp client.
The scenario that we have is the following:
From the upsteam connection we get a request which is replied with a steam of data from the target. All works fine using fasthttp and the customStream body. We simply copy from the target reader to the upstream writer. Now the upstream suddenly closes the TCP connection and we get an error while trying to copy our data from the target.
Now we definitelly want to close the still open connection to the target. The only thing we can do from within the handler is set
fastRequest.SetConnectionClose()andfastResponse.SetConnectionClose(). Though, the TCP connection will not be closed since it is too late when we set those values. The TCP connection is released and eventually re-used by the next upstream connection which would then result into an unexpected error.If the closeConn check is done again in the newCloseReader wrapper function we could set the ConnectionClose() property from the outside and the connection will be closed once we release our request handler.