fix issue #724 & #711 - two threads attempt to use the same database connection & prepared statements

[crmo]

I write a demo to recurrent issue #724 & #711：

    NSString *dbPath = [docPath stringByAppendingPathComponent:@"test.sqlite"];
    _queue = [FMDatabaseQueue databaseQueueWithPath:dbPath];
    for (int i = 0; i < 10; i++) {
        dispatch_async(dispatch_get_global_queue(0, 0), ^{
            [_queue inDatabase:^(FMDatabase * _Nonnull db) {
                FMResultSet *result = [db executeQuery:@"select * from test1 where a = '1'"];
                if ([result next]) {
                }
                // do not close FMResultSet
//                [result close];
            }];
        });
    }

I think the crash reason was if user traversed FMResultSet used if ([FMResultSet next]) and not call [FMResultSet close]. When FMResultSet dealloc in AutoreleasePoolPage::pop(), then [FMResultSet close] and sqlite3_reset(_statement) will be called, if user reading or writing to the database currently, the database connection and prepared statements was accessed by multi-threaded.

I suggest to modify this like this, if the user did not close the FMResultSet after the block ends, we close it in current dispatch_queue_t.

[crmo]

The back trace

[groue]

I had a similar issue with GRDB. When SQLite statements are not reset and finalized when the user needs it, the user may end up with SQLite functions not called on the required dispatch queue (uncontrolled deallocation), or statements that retain database locks longer than needed (late statement reset, with unwanted errors SQLITE_ERROR "destination database is in use").

Of course, the user is not always aware of SQLite subtleties, so the user often does not know what he needs (namely, statement cleanup at a correct point in the lifetime of the program).

This PR calls [FMDatabase closeOpenResultSets], with in turn calls both sqlite3_reset and sqlite3_finalize. This sounds like a good and necessary fix IMHO. It would make FMDB more forgiving to users who "forget" to close their result sets (we all did that).

Now, I may want to suggest a change to this PR. FMDB users may use statement caching. In this case, finalizing statements is too much, but a reset would still be helpful.

[ccgus]

@crmo If you could make this problem occur in a unit test, that would be super helpful to me figuring out wether or not this should be fixed in FMDB. Not closing your result sets is a programmer error IMO, which we might be able to clean up, but calling -closeOpenResultSets seems a bit heavy handed.

[crmo]

@ccgus Thanks for your attention, I added a unit test for this problem.

When I readed the FMDBs document, in chapter Executing Queries` , I saw a description of this issue.

You must always invoke -[FMResultSet next] before attempting to access the values returned in a query, even if you're only expecting one:

FMResultSet *s = [db executeQuery:@"SELECT COUNT(*) FROM myTable"];
if ([s next]) {
    int totalCount = [s intForColumnIndex:0];
}

Typically, there's no need to -close an FMResultSet yourself, since that happens when either the result set is deallocated, or the parent database is closed.

So I used if ([s next]) because I just need one value, but application was crash. Is it misleading here?

Usually we will use while ([s next]) to iterate, [FMResultSet close] will called in the end, this is same as call [FMDatabase closeOpenResultSets] in FMDatabaseQueue when block completed. We need to ensure that the FMDatabaseQueue is fully thread safe, or explain the risks in the documentation.

[ccgus]

I don't consider this a bug. If a client is going to open up a result set, it should be closed. And FMDB even prints out: Warning: there is at least one open result set around after performing [FMDatabaseQueue inDatabase:] to give you a hint to that.

You're right that the documentation should be updated to reflect this.

[zhouyantongiosDev]

@crmo Do you have contact information, such as QQ?

[CodeLife2012]

Cached FMStatement access by multi thread.

Root cause maybe gcd autorelease pool pop is not invoked when finish block execution, so temp var is not guarantee released on the serial queue(not check the gcd source code).

[CodeLife2012]

Maybe need to assert on the FMResultSet should dealloc on the private queue.