Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

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

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

bagder opened this issue Jan 14, 2018 · 3 comments
Labels
HTTP/2

Comments

@bagder
Copy link
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
Copy link

dhakiptk commented Jan 16, 2018 •
edited

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
Copy link
Member Author

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 dhakiptk mentioned this issue Jan 18, 2018
Closed
@bagder bagder closed this as completed in a5e6d6e Jan 18, 2018
@dhakiptk
Copy link

dhakiptk commented Jan 23, 2018 •
edited

Thank you very much for including these changes :)

Andersbakken pushed a commit to Andersbakken/curl that referenced this issue Jan 23, 2018
@pm-kartik-mahajan @Andersbakken
http2: don't close connection when single transfer is stopped
1289af5 
Fixes curl#2237
Closes curl#2249
@dhakiptk dhakiptk mentioned this issue Feb 5, 2018
Closed
@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.
Assignees
No one assigned
Labels
HTTP/2
Milestone
No milestone
Development

No branches or pull requests
2 participants
@bagder @dhakiptk