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

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

Coverage Status

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

@bagder
Copy link
Member

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

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

@vpeter4
Copy link

vpeter4 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
Copy link
Contributor

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

@vpeter4
Copy link

vpeter4 commented Apr 4, 2018

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

@cmeister2
Copy link
Contributor

@vpeter4 Perfect, thanks!

@maxdymond
Copy link
Contributor Author

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 sendhttp2pings branch 2 times, most recently from 856cfe1 to edac466 Compare April 19, 2018 07:59
@bagder
Copy link
Member

bagder 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 sendhttp2pings branch 5 times, most recently from 115aac3 to 552c355 Compare June 29, 2018 12:57
@cmeister2
Copy link
Contributor

🎉

@bagder bagder added the feature-window A merge of this requires an open feature window label Jul 2, 2018
@cmeister2
Copy link
Contributor

Docs updated for 7.62.0.

@cmeister2
Copy link
Contributor

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

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

bagder commented Sep 7, 2018

Thanks!

falconindy pushed a commit to falconindy/curl that referenced this pull request Sep 10, 2018
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.
Labels
feature-window A merge of this requires an open feature window
Development

Successfully merging this pull request may close these issues.

None yet

5 participants