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

35 fix periodic disc #47

Merged
merged 15 commits into from
Oct 4, 2018
Merged

35 fix periodic disc #47

merged 15 commits into from
Oct 4, 2018

Conversation

p-goudet
Copy link
Contributor

Fix the check alive process. The current version has a bug in handling of ping packet in case of sending qos = 0 package.

@p-goudet p-goudet self-assigned this Aug 16, 2018
@p-goudet p-goudet mentioned this pull request Aug 16, 2018
oldgreen and others added 15 commits August 16, 2018 14:40
Let the variable really express what it means.  It's current only reader
can't be meaningfully invoked before a CONNECT packet was sent so there is
no need to update the reader's code.
We don't want to let other objects besides the sender to set
last_packet_sent_at.
…fine.

Let the variable really express what it means.  It's current only reader
can't be meaningfully invoked before a CONNACK packet was received so there
is no need to update the reader's code.
We don't want to let other objects besides the handler to set
last_packet_received_at.
…e in

the return value (currently not used in any caller) if we've actually not
sent the packet.
Situation before this change:
If you keep publishing (only) messages with QoS 0 in the time frame of
keep_alive interval, then you update sender.last_packet_sent_at, but you'll
get no response so you'll never update handler.last_packet_received_at.  So
you send no PINGREQ packets in connection_helper.check_keep_alive and
eventually the check "disconnect_timeout_at <= now" in that method triggers
a disconnect.

Fix this by sending PINGREQ packet also if the client haven't recently
received any packet from the server (besides sending PINGREQ packet if the
client haven't recently sent any packet to the server).  Avoid sending more
PINGREQ packets before the client receives a PINGRESP to the first one.
@rgaufman
Copy link

Any updates on this? :)

@p-goudet
Copy link
Contributor Author

I am sorry I have been busy with other project lately. I would try to go back on this as soon as possible and finally merge it. 🙇

@rgaufman
Copy link

rgaufman commented Oct 1, 2018

Any news? :P

@p-goudet
Copy link
Contributor Author

p-goudet commented Oct 2, 2018

I will to try to merge it by the end of this week. I have to review some local changes and check if I include them or not before that 🙇

@rgaufman
Copy link

rgaufman commented Oct 3, 2018

Thank you, that would be great, will test as soon as it's released! :)

@p-goudet
Copy link
Contributor Author

p-goudet commented Oct 4, 2018

The fix seems to be working, however I got some performances issues while the client is disconnected from the server.

Edit: The performance issue seems to be related with other part. I merge this change

@p-goudet p-goudet merged commit 1a3a14a into master Oct 4, 2018
@rgaufman
Copy link

rgaufman commented Oct 4, 2018

Thank you :) - is there an ETA for releasing paho-mqtt-1.0.13?

@p-goudet
Copy link
Contributor Author

p-goudet commented Oct 4, 2018

I don't know yet, very soon I hope :).

@rgaufman
Copy link

rgaufman commented Oct 6, 2018

Thank you, looking forward :)

@rgaufman
Copy link

rgaufman commented Oct 8, 2018

Friendly follow up :P

@rgaufman
Copy link

rgaufman commented Oct 9, 2018

Any update? :)

@p-goudet
Copy link
Contributor Author

This branch has been already merged on the master branch. Other change would be add to the master branch before the release of 1.0.13 version. You may download the source and install the gems by hand to get the latest update.

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.

4 participants