CDTQIndexManager has weak reference to CDTDatastore

[tomblench]

Checklist

• [x] Tick to sign-off your agreement to the Developer Certificate of Origin (DCO) 1.1
• [x] Added tests for code changes or test/build only changes
• [x] Updated the change log file (CHANGES.md|CHANGELOG.md) or test/build only changes
• [x] Completed the PR template below:

Description

Fix lifecycle issues regarding proper opening and closing of resources across CDTDatastore, CDTQIndexManager, CDTDatastoreManager.

Note that the primary motivator here is that the associated object relationship between CDTDatastore and CDTQIndexManager is currently incorrect when facading through index/query methods, and that it is currently possible to obtain multiple CDTQIndexManager instances for the same underlying SQLite database, which leads to incorrect behaviour.

For more details see "Approach" which gives an overview of the design issues.

Fixes #435

Approach

Lifecycle of key objects

CDTDatastore -> CDTQIndexManager

1-to-1 relationship, strong reference via objc_{get,set}AssociatedObject in CDTDatastore+Query.m.

Each index manager is constructed on demand.

CDTQIndexManager -> CDTDatastore

1-to-1 relationship, weak reference via property datastore, therefore no retain cycles. Because reference to CDTQIndexManager can't 'leak' (all methods facade through CDTDatastore+Query), this reference will always be valid.

Note that if CDTQIndexManager is constructed using +managerUsingDatastore or -initUsingDatastore, the above doesn't hold and it is possible for the reference to datastore to be invalid. Even worse, a user could then use a mixture of API calls through the constructed index manager and facaded through the datastore. Therefore this usage is discouraged (all of our current documentation shows index/query usage facaded through datastore).

For the above reason, add a comment in CDTQIndexManager to clarify that it's not public API.

CDTDatastoreManager -> CDTDatastore

1-to-many relationship, strong references via the openDatastores property, which is a map whose keys are the datastore name, and values the datastore itself.

Note that the only way of obtaining a new or existing datastore instance is through the manager.

This is a change to existing functionality and guarantees users will obtain the same reference when multiple calls to -datastoreNamed are made. This is essential for the CDTDatastore -> CDTQIndexManager to work properly. As of now, this is not the case, and multiple calls to -datastoreNamed will result in a new and different CDTDatastore to be constructed each time (although they all point to the same underlying TD_Database). This causes errors as the different index managers will all try to simultaneously open the same underlying sqlite database.

Note that a user could still have a 'dangling' reference to a CDTDatastore, that is they can assign the pointer of a previously constructed datastore, then dealloc its manager. Now they have a datastore they can't delete (they need the manager for this), and worse, they can now obtain a different pointer for the same underlying datastore by constructing a new manager and calling -datastoreNamed. I think we have to be pragmatic and decide we can't protect users from this API abuse.

CDTDatastore -> CDTDatastoreManager

1-to-1 relationship. This is currently held as a strong reference via the property _manager, but will cause retain cycles when combined with the relationship described in the above para. Is not currently used so can be downgraded to a weak reference or removed.

Schema & API Changes

Add new method closeDatastoreNamed on CDTDatastoreManager to allow users with many datastores open to avoid resource (file handle) starvation.

Security and Privacy

No change.

Testing

See added test class CDTLifecycleTests. Test approach as follows:

New instances

• Calls to CDTDatastore+Query.m methods should always obtain the same CDTQIndexManager instance for a given CDTDatastore instance.
• Calls to CDTDatastoreManager -datastoreNamed should always obtain the same CDTDatastore instance for a given datastore name.

Retain count reaching zero

• When dealloc is called on CDTDatastore, dealloc should be called on the CDTQIndexManager held as an associated object.
• When dealloc is called on CDTDatastoreManager, dealloc should be called on all CDTDatastores in openDatastores.
• Should we have tests for TD_Database retain count?

Explicit close

• Explicit close supported and tested via new method call on CDTDatastoreManager

Explicit delete

• When deleting extensions, dealloc should be called on CDTQIndexManager to allow it to close its underlying database for the extensions directory is deleted. (In deleteDatastoreNamed call [_openDatastores removeObjectForKey:name] first). Note that not doing this is the cause of the following message, as reported by customer:

Other test changes

Updated some existing tests which now need an explicit close before re-opening database to reproduce expected behaviour.

Tests run via Rake now run in release mode, as the original bug report shows certain failure modes are only exhibited when run in release (as opposed to debug) mode.

Removed some #ifdef(DEBUG) guards to allow test code to compile in release mode.

Monitoring and Logging

TODO - some debug NSLogs have been left in, these need to be evaluated and turned into "proper" log statements if necessary.

[tomblench]

@ricellis suggested log-grepping strategy in e8472c404fc337ad1a3c201f803c74373a1d19f4 - freopen stderr to a temp file, then mmap the file to check contents.

[ricellis]

@tomblench looks good, as discussed I think it is worth updating the sample app with the close call if it is relevant there. Other query tests likely need some updates too, or at least an issue to track those updates.