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

Remove double check for keepalive #101

Merged
merged 1 commit into from
Mar 13, 2020
Merged

Remove double check for keepalive #101

merged 1 commit into from
Mar 13, 2020

Conversation

Dirk007
Copy link
Contributor

@Dirk007 Dirk007 commented Mar 4, 2020

(at least on MacOS) the call_later-target is executed some milliseconds before the set timeout and therefore the ping will not be sent in a valid time for the broker.

Explanation:
For example: if keepalive is set to 2 seconds timeout, the call will eventually be made after 1.91(+-) seconds. Thus, the first call comes in after 1.91 seconds and the comparison if time.monotonic() - self._last_data_in >= self._keepalive will evaluate to false. The second call will be made after 3.92 seconds - but its already too late to send the ping to the broker then, because the maximum tolerance on broker-side is 1.5 * keepalive.

As the timeout for call_later is already equal to keepalive, we can simply remove the double check inside the _keep_connection to have everything worky.

@Lenka42
Copy link
Collaborator

Lenka42 commented Mar 5, 2020

@Dirk007 thanks for the PR
the idea is, we do not need to send ping request if there is any other packages exchange between client and broket, so self._last_data_in is set in put_package method
But if such an issue occurs in MacOS, maybe check with some kind of coefficient could help? self._last_data_in >= 0.8 * self._keepalive maybe?

@Dirk007
Copy link
Contributor Author

Dirk007 commented Mar 6, 2020

I understand. a coefficient on its own won't help as the second call will be made too late already in some cases I think. How about changing self._keep_connection_callback = asyncio.get_event_loop().call_later(self._keepalive, self._keep_connection) to self._keep_connection_callback = asyncio.get_event_loop().call_later(self._keepalive / 2, self._keep_connection) together with the coefficient?

@Lenka42
Copy link
Collaborator

Lenka42 commented Mar 6, 2020

@Dirk007 I see. Yes, sounds good to me.

@Dirk007
Copy link
Contributor Author

Dirk007 commented Mar 13, 2020

ping :D

@Lenka42 Lenka42 merged commit b66193e into wialon:master Mar 13, 2020
@Lenka42
Copy link
Collaborator

Lenka42 commented Mar 13, 2020

@Dirk007 sorry for delay 🙂
check version 0.6.3

@wdv4758h
Copy link

I'm literally digging the MQTT connection today. My broker keep saying the peer has keepalive timeout, but I do set the keepalive I want. After some checking, the version I'm using (0.6.2) will have chance to not sending ping request due to a small gap for condition. I see the time diff is like 39.993 when keepalive is 40.

I was about to create a PR, then notice you guys already fix it couple days ago!!!
My issue is fixed after upgrading to 0.6.3, thanks!

@wdv4758h
Copy link

Also this fix not only effects MacOS, my issue was happend on GKE with Debian container (python:3.8.2-slim-buster).

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

3 participants