Skip to content

Implement reliable packet receiving logic#52

Open
python36 wants to merge 1 commit intomasterfrom
recvExact
Open

Implement reliable packet receiving logic#52
python36 wants to merge 1 commit intomasterfrom
recvExact

Conversation

@python36
Copy link
Copy Markdown
Owner

This PR introduces a while loop for recvInto to ensure the exact number of bytes is received. This prevents protocol desynchronization caused by partial reads.

Credits: This implementation is based on the code from PR #48 by @centurysys, with some slight modifications and refactoring. Huge thanks to him for identifying this critical issue and providing the initial solution!

@python36
Copy link
Copy Markdown
Owner Author

@centurysys, could you please take a look at this PR and share your thoughts?
I decided not to move pkt to a ref object for now. Keeping it as a plain object avoids extra heap allocations and reduces pressure on the Garbage Collector.
To satisfy Nim's memory safety in the async block, I’ve adjusted the recvExact logic accordingly. Let me know if you have any feedback or suggestions on this implementation!

@centurysys
Copy link
Copy Markdown
Contributor

Hi @python36, thanks for the update and for incorporating the partial read fix!

The approach with looping recvInto to ensure the exact number of bytes looks correct to me, and it should address the protocol desynchronization issue we were seeing.

I also think keeping pkt as a plain object (instead of moving it to a ref) makes sense, especially from a memory and GC pressure perspective.

One thing I’d like to confirm is how this behaves in edge cases:

  • when the connection is closed mid-read
  • or when recv returns 0 / partial repeatedly

As long as those cases are handled cleanly, this looks like a solid improvement.

I'll also try this in my environment (low-performance embedded system) and report back if I see anything unexpected.

@centurysys
Copy link
Copy Markdown
Contributor

@python36, @ThomasTJdev, @zevv
Also, thank you for maintaining nmqtt — it has been essential for my work. Without it, handling async MQTT in Nim would have been quite difficult.

@python36
Copy link
Copy Markdown
Owner Author

Hi @centurysys, thanks for the feedback!
Regarding the edge cases: as you can see in the implementation, if an error occurs (the except block) or if the remote side closes the connection (r == 0), the loop breaks immediately.
In either case, offset will not be equal to len. I’ve added a check right after the loop: if they don't match, the code triggers ctx.close("remote closed connection") and returns. This ensures we don't process incomplete or corrupted packets.
Looking forward to hearing how it performs in your embedded environment!

@centurysys
Copy link
Copy Markdown
Contributor

Hi @python36, thanks for the clarification.

Yes, that behavior makes sense to me.
If the expected number of bytes is not received, closing the connection instead of processing an incomplete packet is the right choice. That should prevent protocol desynchronization.

One small point: it might be useful to distinguish between "remote closed" and other read errors in the log message, as that could help with debugging.

Regarding testing: unfortunately, the original project where I encountered this issue has already been completed, and I no longer have access to the same IoT Hub environment. So it may be difficult to reproduce long-running tests under exactly the same conditions.

That said, I will still try to test it in a similar setup if possible and let you know if I observe anything unexpected.

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