begin should let NSE unregister it self even if it doesn't have an event factory
#1529
Conversation
Signed-off-by: Nikita Skrynnik <[email protected]>
Signed-off-by: Nikita Skrynnik <[email protected]>
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1529 +/- ##
=======================================
Coverage ? 66.99%
=======================================
Files ? 259
Lines ? 12357
Branches ? 0
=======================================
Hits ? 8279
Misses ? 3557
Partials ? 521 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Nikita Skrynnik <[email protected]>
Signed-off-by: Nikita Skrynnik <[email protected]>
Signed-off-by: Nikita Skrynnik <[email protected]>
Signed-off-by: Nikita Skrynnik <[email protected]>
Signed-off-by: Nikita Skrynnik <[email protected]>
Signed-off-by: Nikita Skrynnik <[email protected]>
Signed-off-by: Nikita Skrynnik <[email protected]>
Signed-off-by: Nikita Skrynnik <[email protected]>
Signed-off-by: Nikita Skrynnik <[email protected]>
Signed-off-by: Nikita Skrynnik <[email protected]>
Signed-off-by: Nikita Skrynnik <[email protected]>
Signed-off-by: Nikita Skrynnik <[email protected]>
Signed-off-by: Nikita Skrynnik <[email protected]>
Signed-off-by: Nikita Skrynnik <[email protected]>
NikitaSkrynnik
requested review from
dualBreath,
glazychev-art,
d-uzlov and
bellycat77
| q.Lock() | ||
| _, err := next.NetworkServiceEndpointRegistryServer(ctx).Unregister(ctx, in) | ||
| q.eventCount-- | ||
| if q.eventCount == 0 { | ||
| b.queueMap.Delete(id) | ||
| } | ||
| q.Unlock() | ||
| return &emptypb.Empty{}, err |
There was a problem hiding this comment.
Is there a reason we hold the lock for the whole next.Unregister call?
I would think that we only need it for q.eventCount manipulations and b.queueMap.Delete.
There was a problem hiding this comment.
Yes, there is a reason for this. Next chain elements can manipulate some data and if we have several concurrent Unregister events it can lead to data races. We need mutex for the whole Unregister call to let only one event go further.
There was a problem hiding this comment.
Oh, I have just noticed that you don't use event factory here.
But now I have a different question: what will happen if we get Register call with the same ID as this Unregister call while Unregister is running under this mutex?
|
Fixed in another PR: #1542 |
Description
beginshould let NSE unregister it self even if it doesn't have an event factory. Also changeexpirea bit to distinguish between events coming frombeginand NSE.Issue link
networkservicemesh/sdk-k8s#456
How Has This Been Tested?
Types of changes