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: stale participant concurrent modification exeption [WPB-15340] 🍒 #3280

Merged
merged 2 commits into from
Feb 7, 2025

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Feb 6, 2025

BugWPB-15340 [Android] Crash when hanging up a call in MLS group

This PR was automatically cherry-picked based on the following PR:

Original PR description:



PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

  • Fixed a ConcurrentModificationException crash occurring in leaveMlsConference(), which was triggered when multiple threads attempted to modify staleParticipantJobs simultaneously.
  • The issue was observed when ending a call in a group conversation where different users had mixed MLS and Proteus protocols.

Causes

  • staleParticipantJobs is a ConcurrentMutableMap, but previous implementations did not ensure thread-safe modifications when iterating over and modifying the collection at the same time.
  • When a participant was marked as "stale" (i.e., hasEstablishedAudio = false), a delayed coroutine was scheduled to remove them from the MLS group.
  • leaveMlsConference() iterated over staleParticipantJobs, removing and canceling jobs. However, updateCallParticipants() could simultaneously modify this map, leading to a ConcurrentModificationException.

Solutions

  • Refactored leaveMlsConference() to use the block {} method from ConcurrentMutableMap, ensuring that all modifications are performed in a thread-safe manner.
  • This prevents concurrent iterations and modifications from causing a crash while still allowing multiple coroutines to access and modify staleParticipantJobs safely.

…3278)

* fix: stale participant concurrent modification exeption

* revert private variable
@github-actions github-actions bot added cherry-pick PR is cherry-picking changes from another banch echoes: product-roadmap/bug Work contributing to resolve a bug not critical enough to have raised an incident. type: bug / fix 🐞 👕 size: XS 🚨 Potential breaking changes labels Feb 6, 2025
@Garzas Garzas enabled auto-merge February 7, 2025 06:21
Copy link

sonarqubecloud bot commented Feb 7, 2025

Copy link
Contributor Author

github-actions bot commented Feb 7, 2025

Test Results

3 513 tests  ±0   3 405 ✅ ±0   6m 7s ⏱️ +12s
  603 suites ±0     108 💤 ±0 
  603 files   ±0       0 ❌ ±0 

Results for commit a54dd11. ± Comparison against base commit 20791fd.

♻️ This comment has been updated with latest results.

Copy link
Contributor Author

github-actions bot commented Feb 7, 2025

@Garzas Garzas added this pull request to the merge queue Feb 7, 2025
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.84%. Comparing base (20791fd) to head (a54dd11).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3280      +/-   ##
===========================================
- Coverage    50.84%   50.84%   -0.01%     
===========================================
  Files         1607     1607              
  Lines        58052    58054       +2     
  Branches      5203     5203              
===========================================
+ Hits         29514    29515       +1     
- Misses       26521    26522       +1     
  Partials      2017     2017              

see 4 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20791fd...a54dd11. Read the comment docs.

@datadog-wireapp
Copy link

Datadog Report

Branch report: fix/stale-particpant-crash-cherry-pick
Commit report: 19e5f53
Test service: kalium-jvm

✅ 0 Failed, 3405 Passed, 108 Skipped, 59.32s Total Time

Merged via the queue into develop with commit c7128c5 Feb 7, 2025
23 checks passed
@Garzas Garzas deleted the fix/stale-particpant-crash-cherry-pick branch February 7, 2025 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick PR is cherry-picking changes from another banch echoes: product-roadmap/bug Work contributing to resolve a bug not critical enough to have raised an incident. 🚨 Potential breaking changes 👕 size: XS type: bug / fix 🐞
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants