search

ninenines / gun

HTTP/1.1, HTTP/2, Websocket client (and more) for Erlang/OTP.
ISC License
891 stars 232 forks source link

Add keepalive_tolerance http2 option #294

Closed zuiderkwast closed 1 year ago

zuiderkwast commented 2 years ago

keepalive_tolerance: The number of unacknowledged pings that can be tolerated before the connection is forcefully closed.

When a keepalive ping is sent to the peer, a counter is incremented and if this counter exceeds the tolerance limit, the connection is forcefully closed. The counter is reset whenever a ping ack is received from the peer.

By default, the mechanism for closing the connection based on ping and ping ack is disabled.

As discussed in #254.

zuiderkwast commented 2 years ago

Test not yet added.

Please check if you think this feature makes sense. Then I'll add a test case.

essen commented 1 year ago

Please try to avoid these merge commits [Merge remote-tracking branch 'origin/master' into keepalive_tolerance] they make things much harder for me to merge afterwards. git pull --rebase origin master is the way to go (where origin is this repository, on my machine it's named upstream).

zuiderkwast commented 1 year ago

OK, I'll revert the merge and do a rebase instead. (Some projects prefer merge instead of force-push since it makes it easier to see what's already reviewed and what's new.)

zuiderkwast commented 1 year ago

No merge commit now. However, I squashed all the commits before rebasing, since otherwise I got multiple intermediate merge conflicts when rebasing.

essen commented 1 year ago

No problem, thanks.

zuiderkwast commented 1 year ago

I see that most 2.0 tagged PRs have been merged now. Any chance this one can make it to 2.0?

If not, would you prefer adding an idle_timeout instead?

essen commented 1 year ago

Merged with many tweaks, even went back on some things I said before. Thanks!

essen commented 1 year ago

Hey, there's some issues in the tests for this on macOS and Windows. Can you please take a look? If you open a PR it'll run the tests again.

zuiderkwast commented 1 year ago

It looks like a timing issue. If something is lagging by 50ms, the gun_down comes too early.

The simplest solution would be to increase the times, making the test take longer to run. Perhaps by a factor 10. Do you have a better idea?

This is from one of the latest mac builds on master:

image

essen commented 1 year ago

If it's a timing issue then yes increasing would be fine for the time being. But it's a bit surprising because macOS tends to be the fastest to run tests and have the least amount of issues from it.

zuiderkwast commented 1 year ago

OK, here's a boring PR increasing the timeouts 10 times: #309. Let's see if all builds pass.

Are you in control of the mac machines? Maybe they were temporarily heavily loaded or something....

essen commented 1 year ago

It's possible for Windows since it's a VM on my (beefy) gaming PC. But macOS is CI-only. I'm in control of all machines so if it's not something trivial I'll be able to take a closer look next week.