Skip to content

Commit

Permalink
http2: Fix missing nghttp2_session_send call in Curl_http2_switched
Browse files Browse the repository at this point in the history
Previously in Curl_http2_switched, we called nghttp2_session_mem_recv to
parse incoming data which were already received while curl was handling
upgrade.  But we didn't call nghttp2_session_send, and it led to make
curl not send any response to the received frames.  Most likely, we
received SETTINGS from server at this point, so we missed opportunity to
send SETTINGS + ACK.  This commit adds missing nghttp2_session_send call
in Curl_http2_switched to fix this issue.

Bug: #192
Reported-by: Stefan Eissing
  • Loading branch information
tatsuhiro-t authored and bagder committed Apr 2, 2015
1 parent 2685041 commit 21e82bd
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 3 deletions.
9 changes: 9 additions & 0 deletions lib/http2.c
Expand Up @@ -1062,6 +1062,15 @@ CURLcode Curl_http2_switched(struct connectdata *conn,
return CURLE_HTTP2;
}

/* Try to send some frames since we may read SETTINGS already. */
rv = nghttp2_session_send(httpc->h2);

if(rv != 0) {
failf(data, "nghttp2_session_send() failed: %s(%d)",
nghttp2_strerror(rv), rv);
return CURLE_HTTP2;
}

return CURLE_OK;
}

Expand Down
5 changes: 2 additions & 3 deletions tests/data/test1801
Expand Up @@ -23,7 +23,6 @@ lies!

<datacheck>
HTTP/1.1 101 Switching!

</datacheck>

# listen to the upgrade request!
Expand Down Expand Up @@ -62,9 +61,9 @@ Upgrade: %H2CVER
HTTP2-Settings: AAMAAABkAAQAAP__

</protocol>
# nothing is returned, because no HTTP/2 data nor headers was handled
# CURLE_HTTP2: Send failure: Broken pipe
<errorcode>
56
16
</errorcode>
</verify>
</testcase>

1 comment on commit 21e82bd

@kdudka
Copy link
Contributor

@kdudka kdudka commented on 21e82bd Jul 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the expected error code in test1801 guaranteed to always be the same?

I get CURLE_RECV_ERROR (Recv failure: Connection reset by peer) on Fedora ARM builders whereas CURLE_HTTP2 (Send failure: Broken pipe) everywhere else. Tested with curl-7.43.0 (which includes this fix) and nghttp2-1.1.1.

I came to conclusion that curl works as expected but the test is based on an assumption that is not guaranteed to hold in general. Is my conclusion correct?

Please sign in to comment.