Improve database container read and reset #3365
Conversation
laevandus
added
🐞 Bug
| ) | ||
| extraData = [:] | ||
| var extraData = [String: RawJSON]() | ||
| if !dto.user.extraData.isEmpty { |
There was a problem hiding this comment.
If data is empty, error is logged which can happen when trying to convert deleted DTO
| @@ -1154,6 +1154,53 @@ extension NSManagedObjectContext: MessageDatabaseSession { | |||
| $0.localState = .pendingUpload | |||
| } | |||
| } | |||
|
|
|||
| func loadMessage( | |||
There was a problem hiding this comment.
Moved these under DatabaseSession so that I can use DatabaseSession in async version of the read()
| let changeCounts = context.currentChangeCounts() | ||
| let results = try actions(context) | ||
| if context.hasChanges { | ||
| assertionFailure("Background context is read only, but calling actions() created changes") | ||
| if changeCounts != context.currentChangeCounts() { | ||
| assertionFailure("Context is read only, but actions created changes: (updated=\(context.updatedObjects), inserted=\(context.insertedObjects), deleted=\(context.deletedObjects)") |
There was a problem hiding this comment.
Did not figure out why deleted message is kept tracked by the context which triggered this assert. Just changed it around so that we only trigger it when actually the actions closure changed something.
| // Finally reset states of all the contexts after batch delete and deletion propagation. | ||
| if let writableContext = self?.writableContext, let allContext = self?.allContext { | ||
| writableContext.invalidateCurrentUserCache() | ||
| writableContext.reset() | ||
|
|
||
| for context in allContext where context != writableContext { | ||
| context.performAndWait { | ||
| context.invalidateCurrentUserCache() | ||
| context.reset() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Moved these here because context should be reset after we are done with the batch delete. Otherwise deletes stick around to contexts (because I added NSManagedObjectContext.mergeChanges above). Better to keep them clean when dealing with logout
SDK Size
|
Generated by 🚫 Danger |
SDK Performance
|
| // Finally reset states of all the contexts after batch delete and deletion propagation. | ||
| if let writableContext = self?.writableContext, let allContext = self?.allContext { | ||
| writableContext.invalidateCurrentUserCache() | ||
| writableContext.reset() |
There was a problem hiding this comment.
Did you also tried reseting these context when a delete is performed on write function? (Not sure if this will do anything tho)
There was a problem hiding this comment.
Did not test that. Slightly afraid to do it because we use these contexts for DB observers. Thinking about letting it be 😄
There was a problem hiding this comment.
Out of curiosity, I tested it and it works. Maybe if we still have problems with this in the future we can go back here and apply the following change before the CatabaseContainer.write :
if !self.backgroundReadOnlyContext.deletedObjects.isEmpty {
self.backgroundReadOnlyContext.reset()
}
… where on logout contexts are referencing DTOs
laevandus
force-pushed
the
fix/database-container-read-and-reset
branch
from
befe2cf to
1736ff7
Compare
SDK Size
|
|
testableapple
added
🟢 QAed
🔗 Issue Links
Resolves PBE-5422
🎯 Goal
Fix issues around database read wrappers and context resets
📝 Summary
🧪 Manual Testing Notes
Case 1:
Log in and log out
Case 2:
☑️ Contributor Checklist