Skip to content
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

Fix data race issue with write buffer #347

Merged
merged 1 commit into from
Dec 21, 2023
Merged

Conversation

thedevop
Copy link
Collaborator

@thedevop thedevop commented Dec 16, 2023

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

@werbenhu
Copy link
Member

@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
Copy link
Collaborator Author

@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.

Copy link
Member

@werbenhu werbenhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thedevop you're right! I missed that. Let's address this bug first and then optimize the rest later.

@werbenhu werbenhu self-requested a review December 18, 2023 03:42
@werbenhu
Copy link
Member

werbenhu commented Dec 18, 2023

@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
Copy link
Collaborator Author

@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 mochi-co merged commit 4c68238 into mochi-mqtt:main Dec 21, 2023
3 checks passed
@mochi-co
Copy link
Collaborator

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

@thedevop thedevop deleted the issue_346 branch December 22, 2023 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants