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

Alternative fix for issue 4014, where we always send pings on ROUTER connections #4016

Conversation

sandykellagher
Copy link
Contributor

@sandykellagher sandykellagher commented Apr 3, 2023

Alternative fix for issue #4014: where we always send pings for ROUTER, GATEWAY and LEAF spoke connections.

This is my own original work that I license to the project

[ x] Link to issue, e.g. Resolves #NNN
Documentation added (if applicable)
Tests added
[ x] Branch rebased on top of current main (git pull --rebase origin main)
[ x] Changes squashed to a single commit (described here)
[x] Build is green in Travis CI
[ x] You have certified that the contribution is your original work and that you license the work to the project under the Apache 2 license
Resolves #

Changes proposed in this pull request:

  • in processPingTimer, add explicit test for ROUTER connection, and only hold off sending ping based on receive ping

@sandykellagher sandykellagher requested a review from a team as a code owner April 3, 2023 14:41
server/client.go Outdated
} else if delta := now.Sub(c.ping.last); delta < pingInterval && !needRTT {
// If we received a ping from the other side within the PingInterval then
// there is no need to send a ping.
if delta := now.Sub(c.ping.last); delta < pingInterval && !needRTT {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like it should be any inbound activity not just a ping.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So other inbound data from a client connection will not suppress the pings here, that is the intended behavior correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well no. As per your suggestion on the other PR, in this update the c.ping.last timestamp is used to record both received pings and client data. There is another change to the end of the readLoop routine where c.ping.last is updated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok maybe rename then to in, so c.in.last or c.inbound.last?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are close, and we are prepping to release 2.9.16.

If we can get this merged later today or no later then tomorrow it will make it in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Not 100% sure this is what you meant, but I have changed the ping check to work with a c.lastIn timestamp, which is updated on receiving client data or a ping, and deleted the c.ping.last field.

@sandykellagher
Copy link
Contributor Author

I've made a change so that the c.ping.last timestamp is updated for receive client data as well as pings, and squashed this commit with the previous one. I've also marked the other PR as closed.

…connections, updating c.lastIn timestamp on receiving client data or ping
@sandykellagher sandykellagher force-pushed the Issue_4014_always_ping_ROUTER_connections branch from d7b2ab5 to 5ae83b7 Compare April 5, 2023 12:04
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Thanks!

@derekcollison derekcollison merged commit 8154136 into nats-io:main Apr 6, 2023
1 check passed
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.

None yet

2 participants