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 https://github.com/mattn/go-sqlite3/pull/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.)

[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;
}