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 iteration and map write #96

Closed
wants to merge 1 commit into from

Conversation

muXxer
Copy link
Contributor

@muXxer muXxer commented Sep 2, 2022

PR #90 introduced a regression.

If a client uses QoS 2, it happens that the broker panics with fatal error: concurrent map iteration and map write

fatal error: concurrent map iteration and map write

goroutine 16 [running]:
github.com/mochi-co/mqtt/server.(*Server).ResendClientInflight(0x1400015c000, 0x140000007e0, 0x0)
        mochi-mqtt/server/server.go:912 +0xf0
github.com/mochi-co/mqtt/server.(*Server).resendPendingInflights(0x1400015c000)
        mochi-mqtt/server/server.go:1122 +0x6c
github.com/mochi-co/mqtt/server.(*Server).eventLoop(0x1400015c000)
        mochi-mqtt/server/server.go:220 +0xf4
created by github.com/mochi-co/mqtt/server.(*Server).Serve
        mochi-mqtt/server/server.go:199 +0x70

This is because the GetAll method returns the map itself, instead of a copy.
The lock in GetAll doesn't help, because the map is used outside when the lock is already released.

There are two ways to fix that.
Either we hold the lock while iteration over the map (see ForEach), and use SetWithoutLocking and GetWithoutLocking in the loop, or we iterate over a copy of the map (see ForEachCopy).

I wasn't sure which approach is better, because one solution needs to copy the map, and the other may have more lock contention.

With the fix in this PR, at least the code doesn't panic anymore.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2978190176

  • 65 of 100 (65.0%) changed or added relevant lines in 2 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-1.2%) to 96.61%

Changes Missing Coverage Covered Lines Changed/Added Lines %
server/server.go 16 19 84.21%
server/internal/clients/clients.go 49 81 60.49%
Files with Coverage Reduction New Missed Lines %
server/server.go 1 96.28%
server/internal/clients/clients.go 2 91.28%
Totals Coverage Status
Change from base Build 2878940102: -1.2%
Covered Lines: 2793
Relevant Lines: 2891

💛 - Coveralls

@mochi-co
Copy link
Collaborator

mochi-co commented Sep 2, 2022

Hi @muXxer ! I was able to replicate it using inovex/mqtt-stresser.

Thanks very much for the PR! I found it quite interesting to read and see your approach, and it allowed me to see the issue more clearly. I think you have the general right idea about the problem - with your insights I was appear to have resolve the problem with very minimal changes (I think about 6 lines), so I will make an issue documenting the problem and PR those shortly.

@mochi-co
Copy link
Collaborator

mochi-co commented Sep 2, 2022

This issue has been resolved in https://github.com/mochi-co/mqtt/releases/tag/v1.3.2
@muXxer Thanks again for your help in identifying the cause and solution this issue!

@mochi-co mochi-co closed this Sep 2, 2022
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.

3 participants