-
Notifications
You must be signed in to change notification settings - Fork 209
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 rare crashes when deleting local database content on logout #3355
Conversation
guard self.canWriteData else { | ||
log.debug("Discarding write attempt.", subsystems: .database) | ||
completion(nil) | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a safeguard here for cases where batch delete is in process and some delayed action triggers database write.
// Save just after triggering remove all | ||
container.write { session in | ||
try session.saveChannel(payload: self.dummyPayload(with: .unique), query: nil, cache: nil) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without canWriteData
this would cause a test failure in this test (but sometimes, because this depends on the timing. I got test failures when I repeated the test 100 times)
9913fbd
to
1938e28
Compare
SDK Size
|
SDK Performance
|
SDK Size
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ✅ Just added a couple suggestions, but nothing blocking 👍
@@ -193,6 +119,45 @@ final class DatabaseContainer_Tests: XCTestCase { | |||
} | |||
} | |||
} | |||
|
|||
func test_removingAllData_whileAnotherWrite() throws { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we maybe add a test or try in our Demo App to logout while there is another controller reference alive and then access that controller's data? 🤔 It would be interesting to see if it previously crashed, and now it does not. Although it might be tricky to actually reproduce this 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I should be able to create a test for this.
- Create controller
- Write some data
- Call remove all
- When remove is done, accessing controller content
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test what fails without current changes.
cdfeaa3
to
3a830b4
Compare
3a830b4
to
5561d49
Compare
Quality Gate passedIssues Measures |
🔗 Issue Links
Resolves PBE-4309
🎯 Goal
Fix crashes on logout related to local database access
📝 Summary
NSManagedObjectContext
that data was deleted on the SQL levelDatabaseContainer
write requests while clearing local data🛠 Implementation
Batch delete removes data on the SQL level and does not notify contexts. Contexts can be left into invalid state if we do not manually notify them.
🧪 Manual Testing Notes
Repeat
No crashes or any other odd things should appear.
☑️ Contributor Checklist