Azure / azure-iot-sdk-csharp

A C# SDK for connecting devices to Microsoft Azure IoT services
Other
466 stars 493 forks source link

MQTT-WS splits each PUBLISH in to 2 WebSocket data frames #1814

Closed wpbrown closed 3 years ago

wpbrown commented 3 years ago

Context

Description of the issue

MQTT-WS splits each PUBLISH in to 2 WebSocket data frames (and 2 TCP packets). This doubles the number of packets and could increase latency on highly constrained networks.

Source

The problem stems from DotNetty encoding MQTT messages in to 2 buffers.

ClientWebSocketChannel sends each of these buffers to the websocket separately resulting multiple frames.

Code sample exhibiting the issue

SimulatedDevice example set to Mqtt-WS. The header and payload will generate 2 frames.

image

abhipsaMisra commented 3 years ago

Thanks for opening this issue. As you've pointed out, this is a result of the DotNetty implementation that our client sdk uses for Mqtt. This issue would be addressed better in the DotNetty repo: https://github.com/Azure/DotNetty/issues

abhipsaMisra commented 3 years ago

@wpbrown I am not able to transfer this issue directly to the DotNetty repo (since I do not have write permissions on that repo). Would you mind closing this issue here and opening it on the DotNetty repo directly?

wpbrown commented 3 years ago

@abhipsaMisra it's not clear that this a problem in DotNetty. DotNetty providing downstream handlers separate buffers could be intentional. the closest code to the problem is in this repo in ClientWebSocketChannel. that class is taking each buffer and sending it on the websocket separately.

abhipsaMisra commented 3 years ago

Edit: As you've pointed it out in the original issue description - The splitting of an MQTT publish packets into 2 separate buffers is happening inside the dotnetty library: https://github.com/Azure/DotNetty/blob/dev/src/DotNetty.Codecs.Mqtt/MqttEncoder.cs#L234-L273.

As you can see here, the MQTT packets are encoded before being written. For a PUBLISH packet, the topic name and QoS information is first added to the byte buffer to be written, and then depending on the presence of "payload" on the PUBLISH packet, it is added as the next item to be written. While I am not sure why Dotnetty chose to implement it this way, this results in our ClientWebSocketChannel writing them as separate byte buffers.

DotNetty providing downstream handlers separate buffers could be intentional. the closest code to the problem is in this repo in ClientWebSocketChannel. that class is taking each buffer and sending it on the websocket separately.

Can you help me understand what you mean here? The byte buffers are added to the ChannelOutboundBuffer, from where the ClientWebSocketChannel queries for the current byte buffer to be written and if it finds one, it sends it across. Dotnetty providing downstream handlers 2 byte buffers for a single PUBLISH packet results in the ClientWebSocketChannel writing them separately.

Did you mean to say that we should dequeue all available byte buffers from the ChannelOutboundBuffer and write them as a single frame?

abhipsaMisra commented 3 years ago

@wpbrown Did you get a chance to look at the comment above and clarify the original question?

wpbrown commented 3 years ago

@abhipsaMisra sorry. I don't know the reasoning for provided 2 buffers either. It at least provides the option to handle the header and payloads differently, but it does not require it.

As far as I know you would have to give the websocket 1 buffer for it to produce 1 websocket data frame. That would require you to dequeue the buffers as you said. If I'm reading the Mqtt encode right, publish is the only message type that has this behavior of maybe having 2 buffers and all other messages are 1 buffer.

abhipsaMisra commented 3 years ago

Update - I've opened up an issue on the DotNetty repo to get their recommendation on the preferred approach to resolve this. You can follow the issue here: https://github.com/Azure/DotNetty/issues/553

abhipsaMisra commented 3 years ago

We received the following recommendation from the DotNetty library: Retrieve all available messages from the ChannelOutboudBuffer and create a CompositeByteBuffer. Once composed, retrieve all enqueued bytes and write that to the web socket.

abhipsaMisra commented 3 years ago

The fix for this issue has now been released: https://github.com/Azure/azure-iot-sdk-csharp/releases/tag/2021-04-26