Update websocket implementation over MQTT to not send multiple ws frames for a single MQTT packet #1873
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abhipsaMisra
commented
Apr 1, 2021
abhipsaMisra
force-pushed
the
abmisr/mqttWsByffer
branch
from
ff5a2df
to
05deb60
Compare
drwill-ms
approved these changes
Apr 1, 2021
abhipsaMisra
requested review from
azabbasi,
barustum,
bikamani,
jamdavi,
timtay-microsoft and
vinagesh
as code owners
barustum
approved these changes
Apr 5, 2021
barustum
reviewed
Apr 5, 2021
| // - The property ReferenceCount of a byte buffer implementation maintains a counter of the no of messages available for dequeueing. | ||
| // A ReferenceCount of 0 indicates that all messages have been flushed and the buffer can be deallocated. | ||
| // By calling the method Retain() on each byte buffer component added to the CompositeByteBuffer, | ||
| // we increase the ReferenceCount by 1 and mark them as ready for dequeuing. |
There was a problem hiding this comment.
the spelling of dequeuing here is different than the one on line 291. Not sure which one is right :)
There was a problem hiding this comment.
whoops, "dequeuing" (so many extra alphabets just sitting there needlessly)
abhipsaMisra
force-pushed
the
abmisr/mqttWsByffer
branch
from
a1bd21e
to
dfa67d7
Compare
… to not send multiple ws frames for a single MQTT packet
abhipsaMisra
force-pushed
the
abmisr/mqttWsByffer
branch
from
dfa67d7
to
33503fc
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates our websocket implementation over MQTT to send a single ws frame per MQTT packet being sent.
In short - our websocket implementation sent each message dequeued from the ChannelOutboundBuffer individually, which sometimes resulted in multiple messages being sent for a single MQTT packet (eg. PUBLISH packet encodes the topic and payload separately, so they would show up as different tcp packets on the wire). The fix is to dequeue all enqueued messages and consolidate them before writing to websocket.
I have added the comments in the code explaining the fix, let me know if something is unclear and I can update it.
Fix for #1814
This github issue also contains some details: Azure/DotNetty#553