search

mochi-mqtt / server

The fully compliant, embeddable high-performance Go MQTT v5 server for IoT, smarthome, and pubsub
MIT License
1.29k stars 222 forks source link

Fix data race issue with write buffer #347

Closed thedevop closed 11 months ago

thedevop commented 11 months ago

Ensure write buffer is not the one used by Hook. As reported in #346.

werbenhu commented 11 months ago

@thedevop @mochi-co @x20080406 @dgduncan In the current processing logic, regardless of network congestion, packets are first written into the write buffer and then into the conn. Even with only one client at the moment, which sends one packet per second, the packet is still buffered before being written into the connection.

I think we need to reconsider this approach. Is it possible to consider using the write buffer only under specific conditions? For example, conditions such as len(cl.State.outbound) > 25% * MAX or buf.len() > 2K.

At the same time, we also need to consider that WritePacket() is called in both the read and write coroutines of each client, which may involve data races.

I would like to hear your opinions.

thedevop commented 11 months ago

@werbenhu, if there is no congestion and there were no buffer waiting to be flushed, the packets are directly written to the connection without buffering. Using your example of Even with only one client at the moment, which sends one packet per second, the packet is still buffered before being written into the connection., the buffer will not kick in.

werbenhu commented 11 months ago

@thedevop I think encapsulating two WritePacket functions is a good choice—one for handling packets within the outbound channel and another for handling packets from the read goroutine. We only need to use the write buffer when processing packets from the outbound channel, while packets from the read goroutine can be directly written to the conn, which can help avoid data race issues and allow for buffer reuse. This way, the code can also be cleaner.

thedevop commented 11 months ago

@werbenhu, the data race issue was from calling hook after the actual write, where it's not protected by the lock. So having 2 WritePacket functions, we still can not reuse the same buffer for the packet as the buffer for the connection.

Note, during packet encoding, with #343, writing of packet payload is reduced by 1. There maybe more opportunities to optimize.

mochi-co commented 11 months ago

Looks good to me, I love a one-line change :)