-
Notifications
You must be signed in to change notification settings - Fork 255
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
refactor: simplify Network
with Framed<.., Codec>
#825
Conversation
Pull Request Test Coverage Report for Build 8346655192Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. I appreciate the introduction of Framed
!
The PR is definitely a step into right direction but I completely miss the batching topic discussed in #810.
Furthermore this patch removes the previously implemented batch sending of packets that are generated in response to incoming frames. The current implementation queues up to 10 packets before calling flush. This PR flushes the response after every processed incoming frame. I haven't profiled but this definitely has a negative performance impact.
Calling Network::send
results in a SinkExt::send
(impled by Framed
). This is basically a poll_ready
+ start_send
+ poll_flush
. This results in a sendto
or similar sys call per packet.
To get back proper batching of outgoing frames there should made use of SinkExt::feed
in a limited loop + SinkExt::flush
. Think I demonstrated this here.
Is there a follow up to this PR already planned?
Thanks for the suggestions, have opened #826 to resolve this, please review! |
Merging this, no functional changes included in this PR |
* ack incoming publishes if required * remove unused write buffer * update tests
After it's merged, some incoming messages disappear in event loop, please help to check. asyncpubsub_v5 output before the merge:
asyncpubsub_v5 output after merge:
|
* fix: v5 doesn't write outgoing packets onto network #825 (comment) * same for pingreq
Thanks for pointing this out @xiaocq2001 there is the lack of a good testsuite for v5, might be a good issue to open! |
As discussed in #814 this simplifies
Network
usingFramed
andCodec
Type of change
mqttbytes::Error
is no longerClone, Copy, PartialEq, Eq
and 2 new variants, for io and packet size limitNetwork
, which might lead to issues with packets in the buffer being dropped when an error is faced. But this seems like premature-optimization to be frankChecklist:
cargo fmt
CHANGELOG.md
if it's relevant to the users of the library. If it's not relevant mention why.