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

Handling partial MQTT packet writes #75

Merged
merged 6 commits into from
Dec 10, 2021

Conversation

ryan-summers
Copy link
Member

This PR fixes #74 by incorporating changes from #68 with additional safeguards.

Specifically, any attempt to write a packet is now safe guarded by checking that packet writes are acceptable. It's invalid to attempt to send a packet while a partial packet is still being transmitted, as this would corrupt the MQTT connection.

CC @mladedav

src/network_manager.rs Outdated Show resolved Hide resolved
src/network_manager.rs Outdated Show resolved Hide resolved
@ryan-summers ryan-summers requested a review from jordens December 10, 2021 11:58
@ryan-summers ryan-summers merged commit 337ba21 into master Dec 10, 2021
@ryan-summers ryan-summers deleted the rs/issue-74/partial-write-buffering branch December 10, 2021 12:50
@mladedav
Copy link
Contributor

mladedav commented Dec 10, 2021

Thank you for finishing it, sorry for dropping it on you and then disappearing.

All the points you brought up certainly seem valid.

The only thing that comes to mind now is that publishing can return WriteFail in case the write would block. I wonder if we want to handle it the same way as the partial write. Now the user has to check for this error and retry the same message later, if we reuse this solution they just need to check can_publish for the next message.

@ryan-summers
Copy link
Member Author

@mladedav Thanks for pointing out the other issue. I've added it to the issue tracker and submitted a PR.

In the future, I'd recommend posting things like this as an issue - I nearly missed this comment

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.

Partially transmitted packets cause broker disconnection
3 participants