search

eclipse-paho / paho.mqtt.golang

Other
2.77k stars 534 forks source link

fix(ping outstanding and ping request check): check the ping timeout … #682

Closed CaiYueTing closed 3 months ago

CaiYueTing commented 3 months ago

check the ping timeout after the check keepalive

MattBrittan commented 3 months ago

Can you please explain what issue this solves? From a quick look this change would mean that the ping timeout would only be fired when time.Since(lastSent) >= time.Duration(c.options.KeepAlive*int64(time.Second)) AND time.Since(pingSent) >= c.options.PingTimeout wherease the current functionality only requires the second condition to be true (by default PingTimeout is 10s and options.KeepAlive is 30s).

CaiYueTing commented 3 months ago

Hi @MattBrittan, I'm thinking, if data's always transmitted, why do we still need to use ping requests?

Before sending a ping, we should check the time of the last update. If no data has been transmitted within the keepalive interval, then we can send a ping request.

If I have any misunderstandings, please correct me.

MattBrittan commented 3 months ago

The code already does that, you have just moved the code that handles the disconnection when no reply is received. Note that I'm not going to do anything further with this without a more detailed explanation and signed ECA (could not accept the PR without the ECA in any event).

CaiYueTing commented 3 months ago

Got it. I'll close this pull request. Thank you for your explanation. Originally, I just wanted to reduce the pingresp not received, disconnecting occurrences, but after reading the README, I realized my message handler wasn't implemented well.