search

Azure / DotNetty

DotNetty project – a port of netty, event-driven asynchronous network application framework
Other
4.09k stars 977 forks source link

MQTT PUBLISH packet encoder splits the topic and payload into different frames #553

Open abhipsaMisra opened 3 years ago

abhipsaMisra commented 3 years ago

The Mqtt encoder splits the PUBLISH packet into different frames, one containing the topic and QoS information, the other containing an optional payload data.

This causes the downstream socket implementation to send them as different different packets. Since the netty implementation encodes the entire PUBLISH packet into the same byte buffer, is there a particular reason dotnetty chose to implement it this way?

Related issue: https://github.com/Azure/azure-iot-sdk-csharp/issues/1814

abhipsaMisra commented 3 years ago

In order for a downstream library to not send multiple TCP packets for the same PUBLISH packet, there are three options that I can think of:

  1. The DotNetty implementation could be updated to not encode the PUBLISH packet into two separate buffers - one for the payload and another for topic information.
  2. Compare ChannelOutboundBuffer.TotalPendingWriteBytes property to ((IByteBuffer)ChannelOutboundBuffer.Current).GetIoBuffer().Count and use that to validate if this is the terminal byte buffer being sent for the current packet.
    • if true, the current byte buffer being sent is the terminal byte buffer being sent, so call ClientWebSocket.SendAsync() with endOfMessage marked as true.
    • if false, the current byte buffer being sent is not the terminal byte buffer being sent, so call ClientWebSocket.SendAsync() with endOfMessage marked as false.
  3. Have DotNetty expose a dedicated property to indicate if the current byte buffer being sent is terminal, or if there are more pending bytes.

@nayato Do you think option 2 is a logical solution for this issue? Can we rely on ChannelOutboundBuffer.TotalPendingWriteBytes to infer if the current message is the terminal message for the current packet?

nayato commented 3 years ago

Going with option 2 you're still going to have multiple WS data frames on the wire. It's just that not every one of them will be marked with FIN flag. That said current behavior is compliant with MQTT over WS spec ([MQTT-6.0.0-2]). I assume the ask is to not split messages in the first place where possible. I'd suggest to modify DoWrite implementation to consolidate buffers accumulated in channelOutboundBuffer before calling SendAsync.

On option 1: The issue at hand is specific to channel implementation for integration with WebSocket in SDK. Writing out messages in a single buffer would result in an additional copy of message payload - a high cost operation.