Skip to content

Send multiple pings during large downloads, resetting self._ping_t #923

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.

Sign up for GitHub

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

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

Send multiple pings during large downloads, resetting self._ping_t #923

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/paho/mqtt/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2174,6 +2174,8 @@ def loop_misc(self) -> MQTTErrorCode:

return MQTTErrorCode.MQTT_ERR_SUCCESS



def max_inflight_messages_set(self, inflight: int) -> None:
"""Set the maximum number of messages with QoS>0 that can be part way
through their network flow at once. Defaults to 20."""
Expand Down Expand Up @@ -3274,7 +3276,7 @@ def _check_keepalive(self) -> None:
last_msg_in = self._last_msg_in

if self._sock is not None and (now - last_msg_out >= self._keepalive or now - last_msg_in >= self._keepalive):
if self._state == _ConnectionState.MQTT_CS_CONNECTED and self._ping_t == 0:
if self._state == _ConnectionState.MQTT_CS_CONNECTED and (self._ping_t == 0 or now - last_msg_in < self.keepalive):

@PierreF PierreF Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As said in #914 (comment), I think last_msg_in should be renamed and reworked into last_bytes_received. It should count the last time a bytes was received on the network (i.e. as soon as self._sock.recv return a length >= 1).

But when the change of last_bytes_received is done, I think it will meet required the translation of MQTT spec I write in #914:

  • We must still send PINGREQ (or any MQTT packet) every keepalive interval: the or added by this PR will cause a new PINGREQ to be sent (and since PINGREQ update lsg_msg_out, we won't flood PINGREQ)
  • We should not disconnect if bytes arrive on the network: this is the primary purpose of the or added
  • We should still disconnect if nothing arrive from the network: this or is capped by keepalive

try:
self._send_pingreq()

@PierreF PierreF Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

while writting above comment I realized one point which is unclear if it really hold:

since PINGREQ update lsg_msg_out, we won't flood PINGREQ

I'm not totally sure this is true. Because it only true if loop_write is called, which if behind a condition (if self._thread is None and self._on_socket_register_write is None). I do believe however that self._ping_t is always updated (OR we are disconnected / will disconnect, and therefore won't loop here).

So I think we should protect this call to pingreq:

Suggested change
self._send_pingreq()
if self._ping_t == 0 or now - self._ping_t >= self._keepalive:
self._send_pingreq()

In any case, I don't see a protocol requirement to send a PINGREQ less than keepalive after previous one. The only requirement is that we need to send any MQTT packet every keepalive, to ensure broker don't disconnect us. But we are never required to send PINGREQ sooner.

except Exception:
Expand Down
Loading