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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix possible deadlock in CurrentChatUser functions #2074

Merged
merged 1 commit into from Jun 9, 2022

Conversation

b-onc
Copy link
Contributor

@b-onc b-onc commented Jun 8, 2022

馃敆 Issue Links

https://getstream.slack.com/archives/CRB9P6L03/p1654667559058219

馃幆 Goal

A customer has reported that calling addDevice causes a deadlock

馃摑 Summary

Calling addDevice twice, once from background queue and once from main queue causes deadlock:
background queue -> acquires the lock (semaphore), waits on main queue because of performAndWait
main queue -> waits on the lock, cannot perform any work

馃洜 Implementation

EntityDatabaseObserver uses viewContext for its DB operations, both in tests and production code. This is because it needs to be performant.

When you try to access EntityDatabaseObserver.item from background queue, since it's an Atomic, it acquires the lock & calls viewContext.performAndWait (to convert DTO to Model) which waits for main queue to complete some work

If you try to access EntityDatabaseObserver.item from main queue, it'll wait on the lock... which is held by background queue, which is waiting on main queue (because of performAndWait), which is waiting on background queue to release the lock, which is waiting on main queue to complete the work, which is waiting on background queue to release the lock......

We cannot "fix" the bug in EntityDatabaseObserver right away, it's complicated. But the bug happens because we access currentUser, which is currentUserObserver.item in the CurrentUserController. This causes DB operations, which is not required, we already have O(1) access to currentUserId, which is all we need.

The patch in this PR will mitigate this issue for now.

馃И Manual Testing Notes

Paste this in EntityDatabaseObserver_Tests

func test_deadlock() throws {
        let testId: String = .unique
        fetchRequest.predicate = NSPredicate(format: "testId == %@", testId)
        
        // Insert the item matching the predicate
        try database.writeSynchronously {
            let context = $0 as! NSManagedObjectContext
            let new = NSEntityDescription.insertNewObject(forEntityName: "TestManagedObject", into: context) as! TestManagedObject
            new.testId = testId
            new.testValue = .unique
        }
        
        try observer.startObserving()
        
        // Read item.value from background queue
        DispatchQueue.global().async {
            XCTAssertEqual(self.observer.item?.id, testId)
        }
        
        // Sleep main thread for a little time
        // to make sure background thread acquires lock
        // if we don't sleep, main thread acquires lock first & no deadlock occurs
        sleep(1)
        
        // Read item.value from main queue
        XCTAssertEqual(self.observer.item?.id, testId)
    }

鈽戯笍 Contributor Checklist

  • I have signed the Stream CLA (required)
  • Changelog is updated with client-facing changes
  • New code is covered by unit tests
  • This change follows zero 鈿狅笍 policy (required)
  • Comparison screenshots added for visual changes
  • Affected documentation updated (docusaurus, tutorial, CMS)

@b-onc b-onc added 馃悶聽Bug An issue or PR related to a bug 馃寪聽SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK labels Jun 8, 2022
@b-onc b-onc requested a review from a team as a code owner June 8, 2022 14:21
@b-onc b-onc force-pushed the fix-currentUser-unnecessaryfetch branch from 92ce91f to d7bb34a Compare June 8, 2022 14:22
Copy link
Contributor

@evsaev evsaev left a comment

Choose a reason for hiding this comment

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

Can we have a unit test for at least one method?

@b-onc b-onc force-pushed the fix-currentUser-unnecessaryfetch branch from d7bb34a to 831b1cd Compare June 9, 2022 09:53
@b-onc b-onc force-pushed the fix-currentUser-unnecessaryfetch branch from 831b1cd to 80d20d8 Compare June 9, 2022 09:54
@b-onc
Copy link
Contributor Author

b-onc commented Jun 9, 2022

@evsaev Done! they were failing anyway....

Copy link
Contributor

@evsaev evsaev left a comment

Choose a reason for hiding this comment

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

LGTM!

@sonarcloud
Copy link

sonarcloud bot commented Jun 9, 2022

Kudos, SonarCloud Quality Gate passed!聽 聽 Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@b-onc b-onc changed the base branch from develop to release/4.16.0 June 9, 2022 10:53
@b-onc b-onc changed the base branch from release/4.16.0 to develop June 9, 2022 10:54
@b-onc b-onc merged commit 8bacbe8 into develop Jun 9, 2022
@b-onc b-onc deleted the fix-currentUser-unnecessaryfetch branch June 9, 2022 10:56
evsaev pushed a commit that referenced this pull request Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
馃悶聽Bug An issue or PR related to a bug 馃寪聽SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants