Datastore clear() async implementation #353
Conversation
wooj2
requested review from
palpatim and
drochetti
and removed request for
palpatim
wooj2
force-pushed
the
master-deletePredicate
branch
from
913c059 to
3f38952
Compare
wooj2
force-pushed
the
master-deletePredicate-clearDatastore
branch
2 times, most recently
from
7b057d6 to
3e7bca7
Compare
| @@ -82,13 +84,15 @@ extension AWSDataStorePlugin: DataStoreBaseBehavior { | |||
| public func delete<M: Model>(_ modelType: M.Type, | |||
| withId id: String, | |||
| completion: @escaping DataStoreCallback<Void>) { | |||
| reinitStorageEngineIfNeeded() | |||
There was a problem hiding this comment.
Instead of doing it lazy-init-style here, can you do it eagerly, right before your completion(.successfulVoid) calls inside of clear(...)?
There was a problem hiding this comment.
Hmm! I would have liked to do what you suggested.. hmmm but...
Based on the implementation of reinitStorageEngineIfNeeded(), I don't think this is what we want, because then we would be calling storage.startSync() which would attempt to restart the entire SyncEngine (so... then we'd potentially rehydrate the data base we just deleted)
If reinitStorageEngine were to be changed to only resolveTheStorageEngine (and not start the sync engine), we'd still have to call to startSyncEngine on any query/delete/subscription. At this point, i'm not seeing a huge benefit one way or the other.
wooj2
force-pushed
the
master-deletePredicate-clearDatastore
branch
2 times, most recently
from
c56df02 to
9020a68
Compare
| } | ||
| self.dataStorePublisher = nil | ||
|
|
||
| if let error = sError { |
There was a problem hiding this comment.
Why not just invoke completion(result)? You should then be able to get rid of L141-147 as well
There was a problem hiding this comment.
WHOA! How did i not see that?! Thank you!
updated to:
self.storageEngine = nil
if #available(iOS 13.0, *) {
if let publisher = self.dataStorePublisher as? DataStorePublisher {
publisher.sendFinished()
}
}
self.dataStorePublisher = nil
completion(result)
| completion(.failure(causedBy: DataStoreError.invalidDatabase(path: "Database path not set", nil))) | ||
| return | ||
| } | ||
| connection = nil |
There was a problem hiding this comment.
I don't remember the SQLite API, but is there a cancel or teardown that we should invoke before releasing our connection?
There was a problem hiding this comment.
I looked in the following link, but didn't see anything:
https://github.com/stephencelis/SQLite.swift/blob/master/Documentation/Index.md
Hmm but, the way I arrived at this approach (connection = nil) is that i noticed that we needed to a call to sqlite3_close() -- which seems to be in the deinit of the Connection class. So I figured this was how to 'teardown' the connection.
I'm going to leave this as-is for now.
wooj2
force-pushed
the
master-deletePredicate-clearDatastore
branch
from
d5a3921 to
60c722b
Compare
wooj2
changed the title
. clear() is an async interface
. After calling clear() on DataStore, the datastore is in an idle state -- that is, the remote sync engine has stopped, all subscriptions have been severed and any publishers the customer may have been listening were called with receiveCompletion() (meaning, they will need to re-subscribe)
. Any subsequent call to query, save, delete, publisher() will restart the sync engine and publishers, mirroring JS's behavior
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.