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

housekeeping: Add housekeeping for connections #1641

Closed
wants to merge 1 commit into from

Conversation

@maxdymond
Copy link
Contributor

commented Jul 4, 2017

Add functionality so that protocols can do custom keepalive on their connections, when an external API function is called.

I'm not expecting this pull request to make it in as is - I've created this to discuss the direction going forward.

@coveralls

This comment has been minimized.

Copy link

commented Jul 4, 2017

Coverage Status

Coverage decreased (-0.2%) to 75.166% when pulling 6ad9973 on maxdymond:sendhttp2pings into 7121a99 on curl:master.

@bagder

This comment has been minimized.

Copy link
Member

commented Jul 7, 2017

Some thoughts so far:

  • consider a function for the multi API too that accepts a CURLM * handle
  • I would like to see this "housekeeping" done automatically also while a multi handle is in use, for when there are idle connections in the cache to keep alive
  • I would prefer the name of the function to hint about it being about connections or the connection cache rather than "housekeeping"
  • the travis error happens because you have to add new functions at the bottom of the header file for some particular platforms' sake
  • an option to set the keep-alive interval time, and I would imagine there could also be a time for a maximum time to keep an unused connection alive
  • docs
  • future expansion: the ping-pong protocols could actually do NOOP commands or similar to achieve a similar effect
@cmeister2

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2017

Naming: "curl_conn_upkeep" has been discussed on IRC and could be a good option.

@vpeter4

This comment has been minimized.

Copy link

commented Feb 17, 2018

I have added a function for the multi API and have it running. Also setting for keep-alive interval time. Thanks to this work by @maxdymond.

@bagder: Can you explain little more your second point above and how it should work?

@cmeister2

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2018

@vpeter4: I seem to have lost your patch; any chance you could link it here please? 😄

@vpeter4

This comment has been minimized.

Copy link

commented Apr 4, 2018

You mean https://pastebin.com/ZrNwwEa8 ? It is everything I used (it includes your changes too).

@cmeister2

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2018

@vpeter4 Perfect, thanks!

@maxdymond maxdymond force-pushed the maxdymond:sendhttp2pings branch from 6ad9973 to e165edf Apr 18, 2018

@maxdymond

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2018

I freshened this pull request.

@bagder: regarding your comments:

  • consider a function for the multi API too that accepts a CURLM * handle
  • I would like to see this "housekeeping" done automatically also while a multi handle is in use, for when there are idle connections in the cache to keep alive
    • I would like to do these, but looking at the curl multi code, I haven't the faintest idea where I'd add such code in in a safe manner. Any thoughts? Otherwise, I'd prefer it if we could defer this feature request to a later date?
  • I would prefer the name of the function to hint about it being about connections or the connection cache rather than "housekeeping"
    • Now refers to "connection upkeep".
  • the travis error happens because you have to add new functions at the bottom of the header file for some particular platforms' sake
    • Hopefully fixed in the latest commit
  • an option to set the keep-alive interval time, and I would imagine there could also be a time for a maximum time to keep an unused connection alive
    • Added an option to set the interval time from @vpeter4's code.
  • docs
    • Can you possibly go into more detail here? I think this needs docs for the new API functions and new CURLOPT value - anything else? An example?
  • future expansion: the ping-pong protocols could actually do NOOP commands or similar to achieve a similar effect

@maxdymond maxdymond force-pushed the maxdymond:sendhttp2pings branch 2 times, most recently from 856cfe1 to edac466 Apr 18, 2018

@bagder

This comment has been minimized.

Copy link
Member

commented Jun 1, 2018

I'd prefer it if we could defer this feature request to a later date?

Sure, I have no problems with taking baby steps. We can take that on in a follow-up work.

docs
Can you possibly go into more detail here? I think this needs docs for the new API functions and new CURLOPT value - anything else? An example?

I was primarily thinking of the mandatory pieces: a man page for the function and setopt that describes what they do and why someone would want to use them, and these man pages should ideally include at least a small example snippets (as all our man pages should).

Of course I would consider a separate example code for docs/examples/ a great addition as well. Supposedly we would add at least one test case for this feature anyone so code has to be written that uses it, which then could be the basis for an example... The example doesn't have to be included in the original PR either, we can work on that separately after the feature lands.

@cmeister2 cmeister2 force-pushed the maxdymond:sendhttp2pings branch 5 times, most recently from 115aac3 to 552c355 Jun 27, 2018

@cmeister2

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

🎉

@maxdymond maxdymond force-pushed the maxdymond:sendhttp2pings branch from 552c355 to b049588 Jul 11, 2018

@cmeister2

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2018

Docs updated for 7.62.0.

@maxdymond maxdymond force-pushed the maxdymond:sendhttp2pings branch from b049588 to 3848cbe Sep 6, 2018

@cmeister2

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2018

Rebased for 7.62.0. I set the dates as 31/Oct as I believe that's the correct date!

@maxdymond maxdymond force-pushed the maxdymond:sendhttp2pings branch from 3848cbe to de02d13 Sep 6, 2018

upkeep: Add a connection upkeep API
Add functionality so that protocols can do custom keepalive on their
connections, when an external API function is called.

Add docs for the new options in 7.62.0

@maxdymond maxdymond force-pushed the maxdymond:sendhttp2pings branch from de02d13 to a23d6f2 Sep 6, 2018

@bagder bagder closed this in 7b655fc Sep 7, 2018

@bagder

This comment has been minimized.

Copy link
Member

commented Sep 7, 2018

Thanks!

falconindy added a commit to falconindy/curl that referenced this pull request Sep 10, 2018
upkeep: add a connection upkeep API: curl_easy_conn_upkeep()
Add functionality so that protocols can do custom keepalive on their
connections, when an external API function is called.

Add docs for the new options in 7.62.0

Closes curl#1641

@lock lock bot locked as resolved and limited conversation to collaborators Dec 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.