fix: concurrent map write cause by concurrent invocation of function … #2168
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2168 +/- ##
==========================================
+ Coverage 37.38% 37.66% +0.28%
==========================================
Files 94 94
Lines 7971 8007 +36
==========================================
+ Hits 2980 3016 +36
Misses 4597 4597
Partials 394 394 ☔ View full report in Codecov by Sentry. |
|
The struct on which unmarshal is being performed is created with new() everytime when DeepCopy is called. Which means the data which is being written is not shared between function calls, so how can there be concurrent writes? I am trying to understand that right now. |
|
Because we have the same kind of implementation as there are in kubernetes repos and nowhere do they use a lock |
|
@liushv0 Can you answer the above questions to help me review this PR? |
|
sorry for late :( apisix-route sync goroutine（gr1）will get test-ar from MemDB， now gr1 begin to sync CR test-apc，func at the same time， gr2 begin to sync CR test-apc，func Plugins in test-apc and Plugins in test-ar pointer to same instance，so two goroutine concurrent marshal this Plugins instance，and panic |
|
@liushv0 I think there is no point in having locks as well as DeepCopy. I think this line need to be removed . This line may have been added when this file was autto generated. |
|
@liushv0 Are you available to make changes here? |
| @@ -113,6 +127,8 @@ func (c *dbCache) GetUpstream(id string) (*v1.Upstream, error) { | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| c.upstreamLock.Lock() | |||
| defer c.upstreamLock.Unlock() | |||
There was a problem hiding this comment.
@liushv0 I think there is no point in having locks as well as DeepCopy. I think this line need to be removed
. This line may have been added when this file was autto generated.There was a problem hiding this comment.
I guess remove this line is effective，and less change，
but honesty，I am not exactly understand the intention of this line，set *out = *in for what。
other side，this line is generated by code-gen tools，maybe somebody will rerun code-gen tools，and overwrite the modification。
that is why I choose use locks。
|
@liushv0 Can you modify this PR to remove locks and instead apply this suggestion?#2168 (comment) |
…DeepCopy (#2167)
Type of change:
What this PR does / why we need it:
Pre-submission checklist: