chore: make AccountManager concurrency safe - WPB-23570#4340
chore: make AccountManager concurrency safe - WPB-23570#4340samwyndham wants to merge 8 commits intodevelopfrom
AccountManager concurrency safe - WPB-23570#4340Conversation
Test Results0 tests 0 ✅ 0s ⏱️ Results for commit a6646e3. ♻️ This comment has been updated with latest results. Summary: workflow run #22709409675 |
| /// such as the accounts users name, | ||
| /// team name if there is any, picture and uuid. | ||
| public final class Account: NSObject, Codable { | ||
| public final class Account: NSObject, Codable, @unchecked Sendable { |
There was a problem hiding this comment.
suggestion: add a comment explaining why @unchecked Sendable is ok here.
| loginCredentials = account.loginCredentials | ||
| handle = account.handle | ||
| backendName = account.backendName | ||
| lock.withLock { |
There was a problem hiding this comment.
thought: I'm wondering if it's feasible to make Account a immutable structure and to modify this method to return a mutated copy:
public func updateWith(_ account: Account) -> Account {
guard userIdentifier == account.userIdentifier else { return
return Account(
userIdentifier: userIdentifier,
userName: account.userName,
teamName: account.teamName ?? self.teamName,
...
)
}There was a problem hiding this comment.
@johnxnguyen It will require wider changes because Account is relied upon to be a reference type - E.g. it is expected to be mutated in one place and those mutations to be relied upon in another place.
samwyndham
requested review from
johnxnguyen and
jullianm
and removed request for
David-Henner
|
3 participants
Issue
This PR makes
AccountManager,AccountStore&AccountSendableand therefore thread safe. It is part of a larger effort to make Session management safer across concurrent contexts.I intentionally chose to do this with the aid of locks instead of Actors or @mainactor annotation as:
awaitkeyword.The negative is there is some additional complexity added to these types themselves.
Note:
I tried to add some tests checking for threading issues but was unsuccessful. I could do it by enabling the thread sanitizer in the test plan and having tests fail on error but there were too many existing tests that would fail in such cases.
Testing
Checklist
[WPB-XXX].UI accessibility checklist
If your PR includes UI changes, please utilize this checklist: