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: infinite loop and code optimization - WPB-16115 #2563

Open
wants to merge 11 commits into
base: release/cycle-3.118
Choose a base branch
from

Conversation

jullianm
Copy link
Contributor

@jullianm jullianm commented Feb 19, 2025

WPB-16115

Issue

Context: When logging out from an MLS account, the account is not deleted because the dispatchGroup notify method still has ongoing operations, specifically commitPendingProposals related work which fills in the dispatch group abnormally. Also we have sometimes an infinite loop when trying to recover after failing to commit pending proposals.

Causes: First reason is that some pending proposals are planned for the future and we do a Task.sleep to wait for that date to commit the proposal, which blocks the groups.
Another reason is that when we retry on a commit failure, we perform a quick sync which (when finishing) triggers the commit pending proposals method again which fills the group again and so on which ends up in a loop.

Proposed solutions: first, use dedicated Tasks to not block the group so we can leave it in a reasonable time and second, when we try to recover from a commit error and performing a quick sync, do not try to commit pending proposals again as we're already in the process of recovering these commits.

This PR is tightly related to this PR which solely focused on fixing the logout issue while this current PR introduces various code optimization related to the logic of committing pending proposals as well as fixing the loop.

Testing

No infinite syncing occurs on MLS account

Checklist

  • Title contains a reference JIRA issue number like [WPB-XXX].
  • Description is filled and free of optional paragraphs.
  • Adds/updates automated tests.

@jullianm jullianm changed the base branch from develop to release/cycle-3.118 February 19, 2025 11:28
@jullianm jullianm marked this pull request as draft February 19, 2025 11:40
@jullianm
Copy link
Contributor Author

Converting it back to draft, I've tested again after merging some new work done in 3.118 and now it fails again.. @netbe this won't make it for play test I need to see what's wrong

@johnxnguyen johnxnguyen removed their request for review February 19, 2025 11:52
@@ -97,7 +97,7 @@ actor EventProcessor: UpdateEventProcessor {

guard !DeveloperFlag.ignoreIncomingEvents.isOn else { return }

let publicKeys = try? self.earService.fetchPublicKeys()
let publicKeys = try? await self.earService.fetchPublicKeys()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: something changed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we were not using await: simple compilation warning but this will be an error in Swift 6

@jullianm jullianm changed the title fix: MLS log out error - WPB-15774 fix: infinite loop and code optimization - WPB-16115 Feb 21, 2025
@jullianm jullianm marked this pull request as ready for review February 21, 2025 14:05
@jullianm jullianm requested a review from netbe February 21, 2025 14:10
Copy link
Contributor

github-actions bot commented Feb 21, 2025

Test Results

    4 files      4 suites   8m 17s ⏱️
4 953 tests 4 948 ✅ 2 💤 3 ❌
4 953 runs  4 951 ✅ 2 💤 0 ❌

For more details on these failures, see this check.

Results for commit ef71844.

♻️ This comment has been updated with latest results.

@datadog-wireapp
Copy link

datadog-wireapp bot commented Feb 21, 2025

Datadog Report

Branch report: fix/mls-log-out-error
Commit report: 5f17862
Test service: wire-ios-mono

✅ 0 Failed, 4764 Passed, 2 Skipped, 4m 41.74s Total Time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants