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 concurrent map access for clients and inflights causes data race #99

Merged
merged 2 commits into from
Sep 2, 2022

Conversation

mochi-co
Copy link
Collaborator

@mochi-co mochi-co commented Sep 2, 2022

As per #98 and #96 - a data race was occurring when the primary server routine and client routines attempted to access the client inflight messages map at the same time, causing fatal errors.

The issue was reproduced placing the server under heavy load with inovex/mqtt-stresser, using qos=2 for both the publisher and subscriber (see #98).

This PR fixes the issue by returning reference maps of the original values, values in the case of inflight messages, pointers to clients in the case of the client map.

  • The inflight messages are never modified by reference once set, so it is reasonable and more cost effective to copy the whole value.
  • The clients are passed by reference, and have more sophisticated locking mechanisms and isolation.

Profiling the solution before and after show a small but negligible reduction in mallocs. When running the above mentioned stress tests, data races or crashes are detected are no longer detected.

With many thanks to @muXxer for identifying the issue and providing a potential solution in #96 :)

Additionally, this PR increases the size of the inline publishing buffer in light of #95.
Finally, error message var naming in clients_test has been updated to better meet code standards.

@mochi-co mochi-co added the bug Something isn't working label Sep 2, 2022
@mochi-co mochi-co self-assigned this Sep 2, 2022
@coveralls
Copy link

Pull Request Test Coverage Report for Build 2981567678

  • 13 of 13 (100.0%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.04%) to 97.843%

Files with Coverage Reduction New Missed Lines %
server/server.go 1 96.64%
Totals Coverage Status
Change from base Build 2878940102: 0.04%
Covered Lines: 2767
Relevant Lines: 2828

💛 - Coveralls

@mochi-co mochi-co merged commit b2fc287 into master Sep 2, 2022
@mochi-co mochi-co deleted the fix-inflight-race branch September 2, 2022 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants