Handle HTTP/2 PINGs from servers #1521
Conversation
|
@maxdymond, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @captain-caveman2k and @dfandrich to be potential reviewers. |
|
@bagder: not sure what the process for requesting a review is. |
| * Function to check on various aspects of a connection. | ||
| */ | ||
| static unsigned int rtsp_conncheck(struct connectdata *check, | ||
| unsigned int checks_to_perform) |
There was a problem hiding this comment.
Since there's only one check now and has only really been one check in a long time, I think you can remove the second argument here to simplify. If we add another check in the future, we can add an argument then!
There was a problem hiding this comment.
I added this because I plan to use it for the keepalive code (which I've already written in a private branch) and hopefully it's more forward looking. If you feel strongly about removing this then I can remove it for now and re-add it in with the next pull request.
There was a problem hiding this comment.
No, if you have immediate plans to actually use the parameter in your next PR then I don't think you need to remove it.
| } | ||
| else if(sval & CURL_CSELECT_IN) { | ||
| /* readable with no error. could still be closed */ | ||
| ret_val = !Curl_connalive(check); |
There was a problem hiding this comment.
My general comment about this approach is that this will effectively prevent libcurl from detecting that h2 connections have died in many cases. It really needs the next step of treatment too to become a good check. The next step being reading and handling the actual ping (or disconnect). But there can also be other frames than ping...
There was a problem hiding this comment.
Is that true? The standard check up until now has been to call SocketIsDead on the first socket on the connection:
static bool SocketIsDead(curl_socket_t sock)
{
int sval;
bool ret_val = TRUE;
sval = SOCKET_READABLE(sock, 0);
if(sval == 0)
/* timeout */
ret_val = FALSE;
return ret_val;
}
The only difference that this change is going to make is if the HTTP/2 connection is readable (sval & CURL_CSELECT_IN) and Curl_connalive() doesn't determine that the connection is dead. From my reading of Curl_connalive() it ought to be pretty reasonable at working out if a FIN has been received - are there any other scenarios that I'm missing here where the connection could be dead but Curl_connalive declares it alive?
There was a problem hiding this comment.
You're right, I had forgotten the MSG_PEEK check there so the situation is better than what I stated above...
|
Any more thoughts on my responses here? |
|
Have you given any thoughts on if this can be tested somehow in the curl test suite? Have you tested this PR in any other way? |
|
I've tested it on my local machine - I have a Python script which sends an HTTP/2 PING on a connection 10 seconds after serving a GET request, and a test client which performs a GET every 30 seconds. Before, the second GET would complain about the connection being dead; now, it doesn't mind and reuses the connection. In terms of getting this tested in the test framework - I tried to run the unit tests for HTTP/2 but I think the tests use nghttpx, which requires a fair few dependencies to compile. Tell you what - I'll try and spin up a VM to see if I can get this tested. I wouldn't want to reduce the code coverage! |
|
I've had a further look at this now and I'm not sure we can test this in a sensible way. Currently the HTTP/2 tests use the Even if we do this, there's no way of checking libcurl to see if it's reusing a connection or not - so there's no way of seeing any problems. I'm not sure where to go. Would it be possible to just get this merged in now, and maybe rethink the HTTP/2 testing strategy in a separate thread? |
|
@bagder - any thoughts on how this could be regressibly tested? |
maxdymond
force-pushed
the
connectionattention
branch
from
77f7de5
to
76c5e8e
Compare
|
(rebase to bring the code back up to date...) |
|
First, please squash the bugfix commit into one of the other commits, we don't need to see your history or merge commits we already know are broken. Then, and I'm not sure why this isn't shown in the CI builds but building debug-enabled HTTP/2-enabled on my machine shows: The function should be made static. |
Add a new type of callback to Curl_handler which performs checks on the connection. Alter RTSP so that it uses this callback to do its own check on connection health.
Add a connection check function to HTTP2 based off RTSP. This causes PINGs to be handled the next time the connection is reused.
maxdymond
force-pushed
the
connectionattention
branch
from
76c5e8e
to
057d927
Compare
|
Ok - I've squashed the fix commit, and the static fix above, into the two functional commits. Not sure why that error didn't come up before - thanks for catching it! |
|
Thanks! |
As per https://curl.haxx.se/mail/lib-2017-05/0096.html, I've made a fix to allow libcurl to handle HTTP/2 PINGs from servers.
The main portion of this fix is to create a new "connection_check" Curl_handler callback, which can be called with flags to check on a connection (Currently, the only check option is CONNCHECK_ISDEAD, to check if the connection is dead or not).
The existing special case (Curl_rtsp_connisdead) is refactored to use this approach, and http2.c is extended to use a cloned version of Curl_rtsp_connisdead.
By default, libnghttp2 handles PING frames automatically (https://nghttp2.org/documentation/nghttp2_option_set_no_auto_ping_ack.html). The PING frames are handled next time a request is made on an existing HTTP/2 connection.