fix: concurrent map iteration and map write #727
Conversation
| for key, val := range r.countMap { | ||
| cts.Counts[key] = val | ||
| } |
There was a problem hiding this comment.
Aren't we just moving the concurrency conflicts just to here? Should we use a mutex to protect this code? @t0rr3sp3dr0 WDYT?
There was a problem hiding this comment.
This should be fine. The problem is that we were passing the reference of the internal Queue map, that is protected by a mutex, to the Counts struct, that is not aware of the mutex. So when someone interacted with the Counts struct, the internal Queue map was being used without locking the mutex. Now the map is being copied to the Counts struct, fixing the problem.
There was a problem hiding this comment.
The Counts struct is not thread-safe and its instances are not being accessed by multiple routines, so we don’t need a mutex that side.
There was a problem hiding this comment.
@luckymrwang, to prevent regressions in the future, can you implement a test to make sure the Current method always returns a copy of the internal map and not a reference to it? Given we are now returning a new map, it’s a good idea to also test if the Counts map contains the same values as the Queue map.
@t0rr3sp3dr0 Ok, I will add it later. |
|
Hi @luckymrwang , |
@JorTurFer @t0rr3sp3dr0 I was busy the past two days and the latest code has been committed. |
|
Statics checks are faillig, could you review them and update changelog as well? |
@JorTurFer The latest code has been committed. |
There was a problem hiding this comment.
Please, add a note in the changelog about this PR: https://github.com/kedacore/http-add-on/blob/main/CHANGELOG.md#fixes
pkg/queue/queue_test.go
Outdated
|
|
||
| err = memory.Resize("host1", 1) | ||
| r.NoError(err) | ||
| memory.Resize("host2", 1) |
There was a problem hiding this comment.
| memory.Resize("host2", 1) | |
| err = memory.Resize("host2", 1) |
Signed-off-by: Cookie Wang <[email protected]>
|
lgtm |
Signed-off-by: Jorge Turrado <[email protected]>
Provide a description of what has been changed
Checklist
README.mddocs/directoryFixes #726