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

Replace DefaultServerCapabilities with NewDefaultServerCapabilities()… #360

Closed thedevop closed 9 months ago

thedevop commented 10 months ago

… to avoid data race

In tests, Capabilities maybe altered. However, DefaultServerCapabilities is a single global instance, where it maybe embedded in running server instances. So when value is altered, tests may fail with data race issue.

Here is an example: In TestServerProcessSubscribeACLCheckDenyObscure: https://github.com/mochi-mqtt/server/blob/65c78534dcb5861dc8ea2163ac58ce603b6e9881/server_test.go#L2790-L2794 and TestServerClearExpiredRetained https://github.com/mochi-mqtt/server/blob/65c78534dcb5861dc8ea2163ac58ce603b6e9881/server_test.go#L3328 are seemingly different tests, but they cause interferences and resulted in data race issue.

This surfaced in the failed test for PR #359.

Note, this is mostly a test issue, as normally one would (should) not alter server capabilities once server started serving.

This may also be the cause for data race issue seen in #200.

coveralls commented 9 months ago

Pull Request Test Coverage Report for Build 7773058128


Files with Coverage Reduction New Missed Lines %
server.go 6 99.14%
<!-- Total: 6 -->
Totals Coverage Status
Change from base Build 7495506564: -0.1%
Covered Lines: 5545
Relevant Lines: 5621

💛 - Coveralls
mochi-co commented 9 months ago

This is a very nice fix, thank you @thedevop. I really like what you've done here.