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

Corrected Cancellation Source for KeepAlive Messages #2129

Merged
merged 8 commits into from
Jan 2, 2025

Conversation

Erw1nT
Copy link
Contributor

@Erw1nT Erw1nT commented Dec 18, 2024

I found out that the issue found in #2108 likely comes due to the fact that the wrong duration is used for checking the keep alive interval.

In this test setup i connect to a public broker and use the approach described in #2108 to see if it is triggered within time.
https://github.com/Erw1nT/MqttKeepAlive/blob/master/Program.cs

Only when this line is added, the connection-changed/disconnected event is fired in time.

.WithTimeout(TimeSpan.FromSeconds(KeepAliveTimeInSeconds))

Copy link
Collaborator

@chkr1011 chkr1011 left a comment

Choose a reason for hiding this comment

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

Please see my comment.

@chkr1011 chkr1011 merged commit 948c7c2 into dotnet:master Jan 2, 2025
1 check was pending
@Erw1nT
Copy link
Contributor Author

Erw1nT commented Jan 6, 2025

@chkr1011 Thank you for reviewing and improving the PR.

@Erw1nT
Copy link
Contributor Author

Erw1nT commented Jan 8, 2025

@chkr1011 We are currently using v4.3.6 in our application and rely on the ManagedMqttClient as of now.
Due to time/budget constraints it would be best for us, if this bugfix would be made available in a v4.3.8 so that we do not have to implement and test a re-connect logic that the ManagedClient offers. And also not handle all other breaking changes...
Is that possible?
Thanks in advance.

@chkr1011
Copy link
Collaborator

I will create a port for version 4 soon.

@Erw1nT
Copy link
Contributor Author

Erw1nT commented Jan 23, 2025

@chkr1011 Have you had any time for the port yet. Would really appreciate it. (I can also see why it's not a high priority ;))

@chkr1011
Copy link
Collaborator

@Erw1nT High priority does not mean it will be done tomorrow. For a project which is maintained by people in their free time, it means it is only one of the first things which is done as soon as the developers have some free time to work on it ;-)

A new PR is here: #2144
Please test latest version 4.3.8 (as soon as available) from myget feed and let me know if it works properly.

@Erw1nT
Copy link
Contributor Author

Erw1nT commented Feb 5, 2025

@chkr1011 It does work as expected now. Thank you for your support.

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.

2 participants