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

fix(keep alive): remove force ping handling #116

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tigercl
Copy link

@tigercl tigercl commented Dec 19, 2020

The original forced ping logic also counted the PINGREQ packets sent by itself, resulting in only one PINGREQ packet sent in two Keep Alive cycles.

@tigercl tigercl requested a review from terry-xiaoyu December 19, 2020 02:35
Copy link
Member

@HJianBo HJianBo left a comment

Choose a reason for hiding this comment

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

We need the appup.src file

@HJianBo HJianBo added the bug label Jan 4, 2021
case ConnMod:getstat(Sock, [send_oct]) of
{ok, [{send_oct, Val}]} ->
OldVal = get(send_oct), put(send_oct, Val),
OldVal == undefined orelse OldVal == Val;
Copy link
Member

Choose a reason for hiding this comment

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

since PING is a one-byte request the correct fix should be to check if OldVal < Val -1 ?

Copy link
Author

Choose a reason for hiding this comment

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

PINGREQ has more than one byte. If we insist on subtracting the length of the PINGREQ ourselves, it is okay, but the force_ping mechanism is useless. It is simplest to send PINGREQ at a fixed time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants