http/2 connection unnecessary closed when single transfer stopped #2237

Closed
bagder opened this Issue Jan 14, 2018 · 3 comments

Comments

Projects
None yet
2 participants
@bagder
Member

bagder commented Jan 14, 2018

I did this

"lali .cpp" reported this issue on the mailing list:

The client gives the server 1 second to respond, the server intentionally
chooses a random value between 0 to 2000 ms to wait before replying to the
client. So sometimes the request doesn't "complete" and so libcurl
"tearsdown" the connection when curl_multi_remove_handle is called(which
ideally it shouldn't for http2)

There are example code provided by the reporter to reproduce the issue in the email.

I expected the following

If the connection is setup, removing an individual HTTP/2 transfer should not close the connection.

curl/libcurl version

libcurl 7.57.0

operating system

Probably not relevant

@bagder bagder added the HTTP/2 label Jan 14, 2018

@dhakiptk

This comment has been minimized.

Show comment Hide comment
@dhakiptk

dhakiptk Jan 16, 2018

patch.txt
I am attaching a patch. Do you think that it makes sense? I am able to verify that it fixes the issues of connection "teardown", but want to be sure that it won't cause problems elsewhere(regression). I did some load testing for a couple of hours and didn't see any memory leaks. Basically the patch says that once we know that the protocol supports "logical streams", even if the transfer didn't reach DONE state, it is sufficient to close/reset the underlying stream(which happens in function Curl_http2_done by calling nghttp2_submit_rst_stream API) and therefore there is no need to "teardown" the underlying TCP connection.

dhakiptk commented Jan 16, 2018

patch.txt
I am attaching a patch. Do you think that it makes sense? I am able to verify that it fixes the issues of connection "teardown", but want to be sure that it won't cause problems elsewhere(regression). I did some load testing for a couple of hours and didn't see any memory leaks. Basically the patch says that once we know that the protocol supports "logical streams", even if the transfer didn't reach DONE state, it is sufficient to close/reset the underlying stream(which happens in function Curl_http2_done by calling nghttp2_submit_rst_stream API) and therefore there is no need to "teardown" the underlying TCP connection.

@bagder

This comment has been minimized.

Show comment Hide comment
@bagder

bagder Jan 18, 2018

Member

Yes, I think it should be fine! @dhakiptk, can you make a PR out of this so that we get some tests run on it before we merge? (if not, I can do it for you)

Member

bagder commented Jan 18, 2018

Yes, I think it should be fine! @dhakiptk, can you make a PR out of this so that we get some tests run on it before we merge? (if not, I can do it for you)

@dhakiptk

This comment has been minimized.

Show comment Hide comment
@dhakiptk

dhakiptk Jan 23, 2018

Thank you very much for including these changes :)

dhakiptk commented Jan 23, 2018

Thank you very much for including these changes :)

Andersbakken added a commit to Andersbakken/curl that referenced this issue Jan 23, 2018

@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.