fix: getCallManagerForClient is not thread safe #2060
Conversation
Datadog ReportAll test runs ✅ 2 Total Test Services: 0 Failed, 0 with New Flaky, 2 Passed Test Services
|
Codecov Report
@@ Coverage Diff @@
## release/candidate #2060 +/- ##
====================================================
Coverage ? 58.04%
Complexity ? 24
====================================================
Files ? 1004
Lines ? 37578
Branches ? 3431
====================================================
Hits ? 21813
Misses ? 14293
Partials ? 1472 Continue to review full report in Codecov by Sentry.
|
MohamadJaara
requested review from
a team,
yamilmedina,
alexandreferris,
borichellow,
Garzas and
trOnk12
and removed request for
a team
| return callManagerHolder.computeIfAbsent(userId) { | ||
| CallManagerImpl( | ||
| calling = calling, | ||
| callRepository = callRepository, | ||
| userRepository = userRepository, | ||
| currentClientIdProvider = currentClientIdProvider, | ||
| selfConversationIdProvider = selfConversationIdProvider, | ||
| callMapper = callMapper, | ||
| messageSender = messageSender, | ||
| conversationRepository = conversationRepository, | ||
| federatedIdMapper = federatedIdMapper, | ||
| qualifiedIdMapper = qualifiedIdMapper, | ||
| videoStateChecker = videoStateChecker, | ||
| conversationClientsInCallUpdater = conversationClientsInCallUpdater, | ||
| kaliumConfigs = kaliumConfigs | ||
| ) |
There was a problem hiding this comment.
Not sure if I understand correctly the fix as computeIfAbsent is not thread safe AFAIK.
Also this way we will not have instance by logged in user.
There was a problem hiding this comment.
Previously, there was a process in place. It first checked if a map contained a "callManager" for a specific user ID. If not, it would create a new "callManager" and save its reference in the map. However, there was a problem. If two threads simultaneously performed this check, they could both conclude that there was no "callManager" for the user and both create a new one.
Now, we've improved this process by using a Java data structure called ConcurrentHashMap. With it, we use the computeIfAbsent method, which ensures that even if multiple threads try to create a "callManager" at the same time, it will be done in a way that is safe and does not result in duplicate "callManagers."
There was a problem hiding this comment.
you can check out what it does here
There was a problem hiding this comment.
as computeIfAbsent is not thread safe AFAIK.
computeIfAbsent in HashMap are not thread safe but we are using ConcurrentHashMap which it is
There was a problem hiding this comment.
but now with this changes, we are no longer adding instances to callManagerHolder
There was a problem hiding this comment.
If the specified key is not already associated with a value, attempts to compute its value using the given mapping function and enters it into this map unless null
MohamadJaara
deleted the
fix/getCallManagerForClient-is-not-thread-safe
branch
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764The PR Description
What's new in this PR?
Issues
GlobalCallManager.getCallManagerForClientis not thread safe, this can cause a case where multiple instances of AVS being createdSolutions
use
computeIfAbsentto fix the issueNeeds releases with:
Testing
Test Coverage (Optional)
How to Test
Briefly describe how this change was tested and if applicable the exact steps taken to verify that it works as expected.
Notes (Optional)
Specify here any other facts that you think are important for this issue.
Attachments (Optional)
Attachments like images, videos, etc. (drag and drop in the text box)
PR Post Submission Checklist for internal contributors (Optional)
PR Post Merge Checklist for internal contributors
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.