make sure Close always removes the runtime finalizer

[charlievieth]

This commit fixes a bug in {SQLiteConn,SQLiteStmt}.Close that would lead to the runtime finalizer not being removed if there was an error closing the connection or statement. This commit also makes it safe to call SQLiteConn.Close multiple times.

This follows up on the work I did in #1301 (remove superfluous use of runtime.SetFinalizer on SQLiteRows) which is why it only addresses the Close methods of SQLiteConn and SQLiteStmt.

[rittneje]

@charlievieth I don't think the change around sqlite3_close_v2 is correct. The SQLite docs say the following:

Calls to sqlite3_close() and sqlite3_close_v2() return SQLITE_OK if the sqlite3 object is successfully destroyed and all associated resources are deallocated.

This implies that if it returns something other than SQLITE_OK, then you can (and should) call it again, although this is more clearly stated for sqlite3_close, since it is documented to return SQLITE_BUSY in some cases.

If the concern is that it is not safe to call sqlite3_close_v2 again if the first call (somehow) fails, I think we need an explicit answer from the SQLite authors on the manner.

However, the change around sqlite3_finalize does appear to be necessary, as the documentation explains that its return value has nothing to do with the finalizer itself succeeding:

If the most recent evaluation of the statement encountered no errors or if the statement is never been evaluated, then sqlite3_finalize() returns SQLITE_OK. If the most recent evaluation of statement S failed, then sqlite3_finalize(S) returns the appropriate error code or extended error code.

(That said, the current s.closed check prevents the second call from having any effect, so this is strictly an optimization.)

[rittneje]

I know it's existing, but I think this check is a bug. The docs for sqlite3_close_v2 say the following:

If sqlite3_close_v2() is called with unfinalized prepared statements, unclosed BLOB handlers, and/or unfinished sqlite3_backups, it returns SQLITE_OK regardless, but instead of deallocating the database connection immediately, it marks the database connection as an unusable "zombie" and makes arrangements to automatically deallocate the database connection after all prepared statements are finalized, all BLOB handles are closed, and all backups have finished.

That means that refusing to call sqlite3_finalize because the database has already been closed will in fact prevent the database from getting properly deallocated.

[charlievieth]

This makes sense, I'll remove the check for dbConnOpen and add some tests around this tomorrow.

[charlievieth]

Removed the check if the db is closed and added a test.

[charlievieth]

@charlievieth I don't think the change around sqlite3_close_v2 is correct. The SQLite docs say the following:

Calls to sqlite3_close() and sqlite3_close_v2() return SQLITE_OK if the sqlite3 object is successfully destroyed and all associated resources are deallocated.

This implies that if it returns something other than SQLITE_OK, then you can (and should) call it again, although this is more clearly stated for sqlite3_close, since it is documented to return SQLITE_BUSY in some cases.

If the concern is that it is not safe to call sqlite3_close_v2 again if the first call (somehow) fails, I think we need an explicit answer from the SQLite authors on the manner.

Thank you for taking a look at this and the thorough review. Both sqlite3_close and sqlite3_close_v2 call the same function sqlite3Close the only difference is that sqlite3_close calls it with the forceZombie argument set to zero, which is why it and only it will return SQLITE_BUSY if the connection is busy - sqlite3_close_v2 skips the connectionIsBusy check.

Basically, sqlite3_close_v2 will only return an error if sqlite3SafetyCheckSickOrOk fails - otherwise it performs all close operations. So us retrying this call in the finalizer will only raise the same error and it should be noted that this condition is checked before any of the actual close logic runs.

From the close docs:

The sqlite3_close_v2() interface is intended for use with host languages that are garbage collected, and where the order in which destructors are called is arbitrary.

I'm hoping that this subtly implies that a single call to sqlite3_close_v2 is sufficient (because given the current implementation it is incredibly hard to determine that it is not - basically, given the current implementation it would be very hard to figure out when you need to call sqlite3_close_v2 again).

Below is the sqlite3 code for reference:

/*
** Two variations on the public interface for closing a database
** connection. The sqlite3_close() version returns SQLITE_BUSY and
** leaves the connection open if there are unfinalized prepared
** statements or unfinished sqlite3_backups.  The sqlite3_close_v2()
** version forces the connection to become a zombie if there are
** unclosed resources, and arranges for deallocation when the last
** prepare statement or sqlite3_backup closes.
*/
SQLITE_API int sqlite3_close(sqlite3 *db){ return sqlite3Close(db,0); }
SQLITE_API int sqlite3_close_v2(sqlite3 *db){ return sqlite3Close(db,1); }

/*
** Close an existing SQLite database
*/
static int sqlite3Close(sqlite3 *db, int forceZombie){
  if( !db ){
    /* EVIDENCE-OF: R-63257-11740 Calling sqlite3_close() or
    ** sqlite3_close_v2() with a NULL pointer argument is a harmless no-op. */
    return SQLITE_OK;
  }
  if( !sqlite3SafetyCheckSickOrOk(db) ){
    return SQLITE_MISUSE_BKPT;
  }
  sqlite3_mutex_enter(db->mutex);
  if( db->mTrace & SQLITE_TRACE_CLOSE ){
    db->trace.xV2(SQLITE_TRACE_CLOSE, db->pTraceArg, db, 0);
  }

  /* Force xDisconnect calls on all virtual tables */
  disconnectAllVtab(db);

  /* If a transaction is open, the disconnectAllVtab() call above
  ** will not have called the xDisconnect() method on any virtual
  ** tables in the db->aVTrans[] array. The following sqlite3VtabRollback()
  ** call will do so. We need to do this before the check for active
  ** SQL statements below, as the v-table implementation may be storing
  ** some prepared statements internally.
  */
  sqlite3VtabRollback(db);

  /* Legacy behavior (sqlite3_close() behavior) is to return
  ** SQLITE_BUSY if the connection can not be closed immediately.
  */
  if( !forceZombie && connectionIsBusy(db) ){
    sqlite3ErrorWithMsg(db, SQLITE_BUSY, "unable to close due to unfinalized "
       "statements or unfinished backups");
    sqlite3_mutex_leave(db->mutex);
    return SQLITE_BUSY;
  }

#ifdef SQLITE_ENABLE_SQLLOG
  if( sqlite3GlobalConfig.xSqllog ){
    /* Closing the handle. Fourth parameter is passed the value 2. */
    sqlite3GlobalConfig.xSqllog(sqlite3GlobalConfig.pSqllogArg, db, 0, 2);
  }
#endif

  while( db->pDbData ){
    DbClientData *p = db->pDbData;
    db->pDbData = p->pNext;
    assert( p->pData!=0 );
    if( p->xDestructor ) p->xDestructor(p->pData);
    sqlite3_free(p);
  }

  /* Convert the connection into a zombie and then close it.
  */
  db->eOpenState = SQLITE_STATE_ZOMBIE;
  sqlite3LeaveMutexAndCloseZombie(db);
  return SQLITE_OK;
}

[rittneje]

Given that the only way for sqlite3_close_v2 to fail is for it to be called more than once (in which case it returns SQLITE_MISUSE), I'm not sure that calling c.lastError() here is safe in general, given that sqlite3_errcode and sqlite3_extended_errcode probably have undefined behavior at that point. It's probably safer to instead directly create an Error that contains only the Code (with explanatory comments).

[charlievieth]

I think it is safe since if sqlite3_close_v2 returns an error then the database is not closed therefore we can still get an extended error message from it. In fact, the sqlite3 CLI does exactly this:

From: sqlite/src/shell.c.in#L5749-L5758

/*
** Attempt to close the database connection.  Report errors.
*/
void close_db(sqlite3 *db){
  int rc = sqlite3_close(db);
  if( rc ){
    eputf("Error: sqlite3_close() returns %d: %s\n", rc, sqlite3_errmsg(db));
  }
}

[charlievieth]

Also, we set SQLiteConn.db to nil regardless of whether or not the call to sqlite3_close_v2 succeeds so that we'll never call it twice (which is what the docs mandate).

[rittneje]

That's sqlite3_close, not sqlite3_close_v2.

[charlievieth]

Calls to sqlite3_close() and sqlite3_close_v2() return SQLITE_OK if the sqlite3 object is successfully destroyed and all associated resources are deallocated.

Yep (which makes sense given the context of the CLI - single-threaded and no GC), but going by the docs any error here means that the database was not fully closed and thus we should be able to safely call sqlite3_errcode and friends with it.

If sqlite3_close_v2() is called with unfinalized prepared statements, unclosed BLOB handlers, and/or unfinished sqlite3_backups, it returns SQLITE_OK regardless, but instead of deallocating the database connection immediately, it marks the database connection as an unusable "zombie" and makes arrangements to automatically deallocate the database connection after all prepared statements are finalized, all BLOB handles are closed, and all backups have finished. The sqlite3_close_v2() interface is intended for use with host languages that are garbage collected, and where the order in which destructors are called is arbitrary.

The real question here and one that I don't know the answer to is: under what circumstances will close fail and set a useful extended error code on the db conn object?

If the answer to that question is never than we can omit the check for the extended error code, but taking a look at the docs it seems a bit ambiguous since they state the conditions under which sqlite3_close_v2 will return SQLITE_OK but not the conditions that will lead to other error codes.

That said, most (if not all code) just wants to know if the close succeeded or failed (then report the error without asserting on the failure cause) so I'm happy to make the change if it gets this PR over the line and revisit this if any issues or further discovery show that we actually should be asking for the extended error code.

[rittneje]

Here too, this may not be generally safe. If the conn was already closed, then lastError() is ill-defined.

Personally, I think this is indicative of a deeper design flaw around this library's error handling. It's kind of silly that we get an error code, then drop it on the floor so we can call sqlite3_errcode instead. Even the extended error code would be simpler if we just configured the conn to give it back in the first place.

I'm going to file another issue to redo the error handling to avoid these issues. But for now, I think you'll have to check if the conn was already closed in this case, and if so directly construct the Error instead of calling lastError.

[charlievieth]

Here too, this may not be generally safe. If the conn was already closed, then lastError() is ill-defined.

It's fine to call lastError() here since the db connection may still be open and the error could be due to something else (which we'd want to capture). Additionally, we know that the statement has not been closed before and we have a test that covers the case of this being called with a closed database.

Personally, I think this is indicative of a deeper design flaw around this library's error handling. It's kind of silly that we get an error code, then drop it on the floor so we can call sqlite3_errcode instead. Even the extended error code would be simpler if we just configured the conn to give it back in the first place.

Agreed, but this would be a breaking change since users might be asserting on sqlite3.Error.Code, which we wouldn't have if we enabled extended error code with: sqlite3_extended_result_codes.

[rittneje]

the db connection may still be open

In Close, we are setting c.db = nil. That means that c.lastError() will no longer work as expected, because it no longer has a reference to the database handle.

Agreed, but this would be a breaking change since users might be asserting on sqlite3.Error.Code

The extended error code is the same as the regular error code, just with more bits. So this is addressed with a bitmask. See https://www.sqlite.org/rescode.html#primary_result_codes_versus_extended_result_codes

[charlievieth]

Actually, there is something here, but it's a bit different. If the conn is already closed then SQLiteConn.lastError() will return nil since we set the conn to nil in SQLiteConn.Close so this is safe, but it means that we drop the error from sqlite3_finalize when the db conn is closed. We previously returned the error "sqlite statement with already closed database connection" if db conn was closed, but removed that check since it was preventing us from calling sqlite3_finalize on the statement.

The fix here is to either preserve the error returned by sqlite3_finalize if the database is already closed or if sqlite3_finalize does return an error we should return the current "sqlite statement with already closed database connection" error. Assuming no one is asserting on that error message the later option might be better since it comes directly from sqlite3.

[charlievieth]

Bit of a race condition on us both commenting, but I think my last comment addresses your first point about Close. As for the extended error code - thank you - I forgot that you can use a bitmask for them.

[charlievieth]

@rittneje After some thought I think you're right and that for this change to work the overall error handling needs to be improved. In commit d3deb2d I updated the error handling to: always use the extended result code; respect the original/offending result code; and safely handle the underlying db connection being closed in lastError. It also, changes the logic so that when sqlite3_close_v2 fails we don't rely on the db (which might be in a weird state) to generate the sqlite3.Error.

Thank you again for your review.