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

make the keep alive interval configurable #3444

Merged

Conversation

nmldiegues
Copy link
Contributor

So far the KeepAlive property for quic.Config allowed only to enable Ping send on inactive quic.Connection. That would happen half-way through the MaxIdleTimeout.

With this change, we make the configuration more flexible, by turning that property into KeepAlivePeriod, which allows to define the period for sending Ping frame since last activity for that quic.Connection.

The value used is the smaller one among:

  • MaxIdleTimeout/2
  • KeepAlivePeriod
  • MaxKeepAliveInterval

Context: We're using quic-go in https://github.com/cloudflare/cloudflared as a substrate for proxying multiplexed application traffic (e.g. TCP/UDP traffic).

Why: We've found situations where our quic.Connections were closed due to no network activity and we believe they should not have, for which having more pings could have potentially have helped during the idle period.

This should not change behaviour for anyone not caring about keep alive.
For those that defined the property, this is a breaking change API-wise (albeit a trivial one to fix). If that's worrisome, we can keep the existing KeepAlive flag and add the new one on the side (where KeepAlive being true defaults to half the idle period unless the KeepAlivePeriod new flag is configured).

@google-cla
Copy link

google-cla bot commented Jun 6, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@marten-seemann marten-seemann self-requested a review June 6, 2022 14:01
@marten-seemann
Copy link
Member

Why: We've found situations where our quic.Connections were closed due to no network activity and we believe they should not have, for which having more pings could have potentially have helped during the idle period.

This is interesting. What was the idle timeout value in those cases, and what keep alive are you planning to use instead?

This PR lgtm, but CI seems to be unhappy.

@nmldiegues
Copy link
Contributor Author

This is interesting. What was the idle timeout value in those cases, and what keep alive are you planning to use instead?

We have 15 sec idle timeout (so Ping at 7.5sec of inactivity).
The plan is to make it 5 sec idle timeout, and ping every 1sec of inactivity.

This PR lgtm, but CI seems to be unhappy.

Ups, I ran the integration tests, but forgot to run the unit tests. Found the culprit and pushed fix.

@codecov
Copy link

codecov bot commented Jun 7, 2022

Codecov Report

Merging #3444 (3a69e9f) into master (990b1fe) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3444   +/-   ##
=======================================
  Coverage   85.69%   85.69%           
=======================================
  Files         135      135           
  Lines        9955     9955           
=======================================
  Hits         8530     8530           
  Misses       1048     1048           
  Partials      377      377           
Impacted Files Coverage Δ
http3/client.go 90.91% <ø> (ø)
config.go 100.00% <100.00%> (ø)
connection.go 77.56% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 990b1fe...3a69e9f. Read the comment docs.

@marten-seemann marten-seemann changed the title Make keep alive configurable make the keep alive interval configurable Jun 9, 2022
@marten-seemann marten-seemann merged commit 4c96cf7 into quic-go:master Jun 9, 2022
@MarcoPolo MarcoPolo mentioned this pull request Jul 7, 2022
41 tasks
@ajnavarro ajnavarro mentioned this pull request Aug 24, 2022
72 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants