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

Handle HTTP/2 PINGs from servers #1521

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@maxdymond
Contributor

maxdymond commented May 31, 2017

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.

@mention-bot

This comment has been minimized.

mention-bot commented May 31, 2017

@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.

@maxdymond

This comment has been minimized.

Contributor

maxdymond commented Jun 1, 2017

@bagder: not sure what the process for requesting a review is.

@bagder bagder added the HTTP/2 label Jun 2, 2017

* Function to check on various aspects of a connection.
*/
static unsigned int rtsp_conncheck(struct connectdata *check,
unsigned int checks_to_perform)

This comment has been minimized.

@bagder

bagder Jun 2, 2017

Member

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!

This comment has been minimized.

@maxdymond

maxdymond Jun 2, 2017

Contributor

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.

This comment has been minimized.

@bagder

bagder Jun 7, 2017

Member

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);

This comment has been minimized.

@bagder

bagder Jun 2, 2017

Member

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...

This comment has been minimized.

@maxdymond

maxdymond Jun 2, 2017

Contributor

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?

This comment has been minimized.

@bagder

bagder Jun 7, 2017

Member

You're right, I had forgotten the MSG_PEEK check there so the situation is better than what I stated above...

@maxdymond

This comment has been minimized.

Contributor

maxdymond commented Jun 6, 2017

Any more thoughts on my responses here?

@bagder

This comment has been minimized.

Member

bagder commented Jun 7, 2017

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?

@maxdymond

This comment has been minimized.

Contributor

maxdymond commented Jun 7, 2017

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!

@maxdymond

This comment has been minimized.

Contributor

maxdymond commented Jun 8, 2017

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 curl binary to issue requests against nghttpx. There's not really a way of overriding this process, so if I wanted custom HTTP/2 behaviour (like injecting PING frames) i'd either have to bundle the Python script (rubbish) or write an HTTP/2 server in C/Perl (unlikely).

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?

@maxdymond

This comment has been minimized.

Contributor

maxdymond commented Jun 14, 2017

@bagder - any thoughts on how this could be regressibly tested?

@maxdymond maxdymond force-pushed the maxdymond:connectionattention branch from 77f7de5 to 76c5e8e Jun 20, 2017

@coveralls

This comment has been minimized.

coveralls commented Jun 20, 2017

Coverage Status

Coverage increased (+0.06%) to 73.844% when pulling 76c5e8e on maxdymond:connectionattention into 0feb762 on curl:master.

@maxdymond

This comment has been minimized.

Contributor

maxdymond commented Jun 20, 2017

(rebase to bring the code back up to date...)

@bagder

This comment has been minimized.

Member

bagder commented Jun 22, 2017

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:

http2.c:162:6: error: no previous prototype for ‘http2_connisdead’ [-Werror=missing-prototypes]
 bool http2_connisdead(struct connectdata *check)
      ^~~~~~~~~~~~~~~~

The function should be made static.

maxdymond added some commits May 31, 2017

handler: refactor connection checking
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.
http2: handle PING frames
Add a connection check function to HTTP2 based off RTSP. This causes
PINGs to be handled the next time the connection is reused.

@maxdymond maxdymond force-pushed the maxdymond:connectionattention branch from 76c5e8e to 057d927 Jun 22, 2017

@maxdymond

This comment has been minimized.

Contributor

maxdymond commented Jun 22, 2017

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!

@coveralls

This comment has been minimized.

coveralls commented Jun 22, 2017

Coverage Status

Coverage decreased (-0.01%) to 73.789% when pulling 057d927 on maxdymond:connectionattention into b404370 on curl:master.

@bagder bagder closed this in c98e97e Jun 30, 2017

@bagder

This comment has been minimized.

Member

bagder commented Jun 30, 2017

Thanks!

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

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