Opening and closing of databases

[benstadin]

Hi,

I think the implementation of Java_org_sqlite_core_NativeDB__1open() and Java_org_sqlite_core_NativeDB__1close()in NativeDB.c is wrong against the behaviour of SQLite databases:

• In open() the db is attempted to be closed when it was already opened. This may lead to odd behaviour (despite the fact that it's a user error to attempt to open the db a second time). First of all I think there should just be an error thrown, but the db should not be be closed.
  Secondly, trying to close a db may or may not be successfull: If there is an unfinalized prepared statement the db will never be closed by SQLite. The result of sqlit3_close() must be checked in any case.
  If this is however for whatever reason intended behaviour that the db is closed, there should rather be sqlite3_close_v2() called, so that the db is closed when the last statement was finalized.
• Similarly in close() the stored db handle must not be unset, and a meaningful error should be thrown (possible SQLite complains are again about not being able to close because of an unfinalized prepared statement, or else). So better leave the db open and keep the handle, until the db was really closed.

So I'd simple remove the sqlite3_close from within the open() method, and change close to this:

JNIEXPORT void JNICALL Java_org_sqlite_core_NativeDB__1close(
JNIEnv *env, jobject this)
{
int err = sqlite3_close(gethandle(env, this));
if (err == SQLITE_OK) {
//clearSpatialiteCacheForHandle(gethandle(env, this));
sethandle(env, this, 0);
}
else {
throw_errorcode(env, this, err);
}
}

Kind regards
Ben

Readups:
https://www.sqlite.org/c3ref/close.html

[benstadin]

Below is an alternative close implementation, using a variation of sanity code that I use in my projects. This close code prints out the SQL of the open statements, attempts to finalize them, and retries that a few (100) times. If it was solely a temporary issue (busy or logged flag, but no open statements) no error is thrown. If there were unfinalized statements, an error is thrown - but the database is closed gracefully then instead of having an unclosed zombie sqlite3 connection.

Please see the "todo" comment below. There should be the SQL logged which wasn't finalized. I didn't implement this yet.

JNIEXPORT void JNICALL Java_org_sqlite_core_NativeDB__1close(
        JNIEnv *env, jobject this)
{
    sqlite3 *handle = gethandle(env, this);
    int err = sqlite3_close(handle);
    if (err == SQLITE_OK) {
        // clearSpatialiteCacheForHandle(gethandle(env, this));
        sethandle(env, this, 0);
    }
    else {
        // Wait while busy for some attempts, and close any prepared statements
        int retry = 0;
        int didFinalizeOpenStatements = 0;
        int numberOfRetries = 0;
        int busyRetryTimeout = 100;
        int shouldFinalizeOpenStatements = 1;
        do
        {
            retry   = 0;
            err     = sqlite3_close(handle);
            // The db is busy or locked, or there are unfinalized prepared statements
            if (SQLITE_BUSY == err || SQLITE_LOCKED == err)
            {
                retry = 1;
                sqlite3_sleep(100);

                if (busyRetryTimeout && (numberOfRetries++ > busyRetryTimeout))
                {
                    throwexmsg(env, "SQLite database busy or locked, tried 100 times but still unable to close");
                    return;
                }
                if (shouldFinalizeOpenStatements)
                {
                    shouldFinalizeOpenStatements = 0;
                    sqlite3_stmt *pStmt;
                    while ((pStmt = sqlite3_next_stmt(handle, 0x00)) != 0)
                    {
                        didFinalizeOpenStatements = 1;
                        // todo: please add log output here - useful for informing user which SQL statement he did not finalize
//                        log = [NSString stringWithFormat:@"Closing leaked statement for SQL: %s", sqlite3_sql(pStmt)];
                        err = sqlite3_finalize(pStmt);
                        if (SQLITE_BUSY == err || SQLITE_LOCKED == err)
                        {
                            // Something interferred again - let's do another round
                            shouldFinalizeOpenStatements = 1;
                            break;
                        }
                    }
                }
            }
            // Any other irrecoverable error occurred (quite rare event)
            else if (SQLITE_OK != err)
            {
                char str[256];
                sprintf(str, "Unhandled SQLite error %d occurred while trying to close database", err);
                throwexmsg(env, str);
                return;
            }
        }
        while (retry);

        if (didFinalizeOpenStatements) {
            throwexmsg(env, "Database was closed while there were open SQLite3 statements. All statements have been \
                       finalized and the db was closed successfully. But you should fix your code to finalize these statements, \
                       crashes and data loss are likely otherwise. See above which SQL statements are affected. ");
        }

        //clearSpatialiteCacheForHandle(gethandle(env, this));
        sethandle(env, this, 0);
    }
}

[xerial]

Thanks for the fix. Could you create a pull request in order to review and test the changes at ease?

[gitblit]

@benstadin If after you review the current state of master you think your change would still be appropriate please open a PR with your proposal. Closing this issue.