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

[CIS-632] Fix race conditions on database observers #796

Merged
merged 2 commits into from Feb 3, 2021

Conversation

VojtaStavik
Copy link
Contributor

@VojtaStavik VojtaStavik commented Feb 3, 2021

We already have a fix like this at other places in the observers. I just extended it to cover the other exposed cases, too.


Fixed exactly as @cardoso suggested :trollface:

Screen Shot 2021-02-03 at 14 40 02

@VojtaStavik VojtaStavik added the 🌐 SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK label Feb 3, 2021
@VojtaStavik VojtaStavik force-pushed the cis-632-fix-crash-in-current-user-controller branch from 4cbce9a to 42d3fbc Compare February 3, 2021 13:41
_item.computeValue = { [weak self] in
// Ideally, this should rather be `unowned`, however, `deinit` is not always called on the same thread as this
// callback which can cause a race condition when the object is already being deinited on a different thread.
guard let self = self else { return nil }
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need [weak self] here, we can get by with [weak frc] and the guard let fetchedObjects = frc?.fetchedObjects will take care of it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@b-onc what do you think about 320a228 ?

@codecov
Copy link

codecov bot commented Feb 3, 2021

Codecov Report

Merging #796 (a6341fb) into main (9f042d0) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #796      +/-   ##
==========================================
+ Coverage   88.62%   88.66%   +0.04%     
==========================================
  Files         219      219              
  Lines        8570     8576       +6     
==========================================
+ Hits         7595     7604       +9     
+ Misses        975      972       -3     
Impacted Files Coverage Δ
...treamChat/Controllers/EntityDatabaseObserver.swift 100.00% <100.00%> (ø)
.../StreamChat/Controllers/ListDatabaseObserver.swift 92.85% <100.00%> (+0.14%) ⬆️
...ests/Mock Network/RequestRecorderURLProtocol.swift 80.55% <0.00%> (+8.33%) ⬆️

Continue to review full report at Codecov.

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

We already have a fix like this on other places in the observers. I just extended it to cover the other exposed cases, too.
@VojtaStavik VojtaStavik force-pushed the cis-632-fix-crash-in-current-user-controller branch from 320a228 to a6341fb Compare February 3, 2021 13:52
@VojtaStavik VojtaStavik merged commit 9a2a96f into main Feb 3, 2021
@VojtaStavik VojtaStavik deleted the cis-632-fix-crash-in-current-user-controller branch February 3, 2021 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌐 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

3 participants