Implement sql::Connection::RazeAndClose().

sql/connection.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sql/connection.h"
 
 #include <string.h>
 
 #include "base/file_path.h"
 #include "base/logging.h"
@@ skipping 58 matching lines @@
   if (number_ != other.number_)
     return number_ < other.number_;
   return strcmp(str_, other.str_) < 0;
 }
 
 ErrorDelegate::~ErrorDelegate() {
 }
 
 Connection::StatementRef::StatementRef()
     : connection_(NULL),
-      stmt_(NULL) {
+      stmt_(NULL),
+      was_valid_(false) {
 }
 
 Connection::StatementRef::StatementRef(sqlite3_stmt* stmt)
     : connection_(NULL),
-      stmt_(stmt) {
+      stmt_(stmt),
+      was_valid_(stmt != NULL) {
 }
 
 Connection::StatementRef::StatementRef(Connection* connection,
                                        sqlite3_stmt* stmt)
     : connection_(connection),
-      stmt_(stmt) {
+      stmt_(stmt),
+      was_valid_(stmt != NULL) {
   connection_->StatementRefCreated(this);
 }
 
 Connection::StatementRef::~StatementRef() {
   if (connection_)
     connection_->StatementRefDeleted(this);
-  Close();
+  Close(false);
 }
 
-void Connection::StatementRef::Close() {
+void Connection::StatementRef::Close(bool forced) {
   if (stmt_) {
     // Call to AssertIOAllowed() cannot go at the beginning of the function
     // because Close() is called unconditionally from destructor to clean
     // connection_. And if this is inactive statement this won't cause any
     // disk access and destructor most probably will be called on thread
     // not allowing disk access.
     // TODO(paivanof@gmail.com): This should move to the beginning
     // of the function. http://crbug.com/136655.
     AssertIOAllowed();
     sqlite3_finalize(stmt_);
     stmt_ = NULL;
   }
   connection_ = NULL;  // The connection may be getting deleted.
+
+  // Forced close is expected to happen from a statement error
+  // handler, in that case maintain the sense of |was_valid_| which

[pkotwicz, 2013/01/31 19:22:00]

Nit: Use a ',' instead of a '.'

[Scott Hess - ex-Googler, 2013/02/05 01:20:25]

I'll assume you meant it the other way around, or to use a ';'.  I'll go with
'.' :-).

+  // previously held for this ref.
+  was_valid_ = was_valid_ && forced;
 }
 
 Connection::Connection()
     : db_(NULL),
       page_size_(0),
       cache_size_(0),
       exclusive_locking_(false),
       transaction_nesting_(0),
       needs_rollback_(false),
       in_memory_(false),
@@ skipping 10 matching lines @@
 #elif defined(OS_POSIX)
   return OpenInternal(path.value());
 #endif
 }
 
 bool Connection::OpenInMemory() {
   in_memory_ = true;
   return OpenInternal(":memory:");
 }
 
-void Connection::Close() {
+void Connection::CloseInternal(bool forced) {
   // TODO(shess): Calling "PRAGMA journal_mode = DELETE" at this point
   // will delete the -journal file.  For ChromiumOS or other more
   // embedded systems, this is probably not appropriate, whereas on
   // desktop it might make some sense.
 
   // sqlite3_close() needs all prepared statements to be finalized.
-  // Release all cached statements, then assert that the client has
-  // released all statements.
+
+  // Release cached statements.
   statement_cache_.clear();
-  DCHECK(open_statements_.empty());
 
-  // Additionally clear the prepared statements, because they contain
-  // weak references to this connection.  This case has come up when
-  // error-handling code is hit in production.
-  ClearCache();
+  // With cached statements released, in-use statements will remain.
+  // Closing the database while statements are in use is an API
+  // violation, except for forced close (which happens from within a
+  // statement's error handler).
+  DCHECK(forced || open_statements_.empty());
+
+  // Deactivate any outstanding statements so sqlite3_close() works.
+  for (StatementRefSet::iterator i = open_statements_.begin();
+       i != open_statements_.end(); ++i)
+    (*i)->Close(forced);
+  open_statements_.clear();
 
   if (db_) {
     // Call to AssertIOAllowed() cannot go at the beginning of the function
     // because Close() must be called from destructor to clean
     // statement_cache_, it won't cause any disk access and it most probably
     // will happen on thread not allowing disk access.
     // TODO(paivanof@gmail.com): This should move to the beginning
     // of the function. http://crbug.com/136655.
     AssertIOAllowed();
     // TODO(shess): Histogram for failure.
     sqlite3_close(db_);
     db_ = NULL;
   }
 }
 
+void Connection::Close() {
+  // Database was already closed via RazeAndClose(), ignore.
+  if (poisoned_)
+    return;
+
+  CloseInternal(false);
+}
+
 void Connection::Preload() {
   AssertIOAllowed();
 
   if (!db_) {
-    DLOG(FATAL) << "Cannot preload null db";
+    DLOG_IF(FATAL, !poisoned_) << "Cannot preload null db";
     return;
   }
 
   // A statement must be open for the preload command to work. If the meta
   // table doesn't exist, it probably means this is a new database and there
   // is nothing to preload (so it's OK we do nothing).
   if (!DoesTableExist("meta"))
     return;
   Statement dummy(GetUniqueStatement("SELECT * FROM meta"));
   if (!dummy.Step())
     return;
 
 #if !defined(USE_SYSTEM_SQLITE)
   // This function is only defined in Chromium's version of sqlite.
   // Do not call it when using system sqlite.
   sqlite3_preload(db_);
 #endif
 }
 
 // Create an in-memory database with the existing database's page
 // size, then backup that database over the existing database.
 bool Connection::Raze() {
   AssertIOAllowed();
 
   if (!db_) {
-    DLOG(FATAL) << "Cannot raze null db";
+    DLOG_IF(FATAL, !poisoned_) << "Cannot raze null db";
     return false;
   }
 
   if (transaction_nesting_ > 0) {
     DLOG(FATAL) << "Cannot raze within a transaction";
     return false;
   }
 
   sql::Connection null_db;
   if (!null_db.OpenInMemory()) {
@@ skipping 69 matching lines @@
 
   // Exactly one page should have been backed up.  If this breaks,
   // check this function to make sure assumptions aren't being broken.
   DCHECK_EQ(pages, 1);
 
   return true;
 }
 
 bool Connection::RazeWithTimout(base::TimeDelta timeout) {
   if (!db_) {
-    DLOG(FATAL) << "Cannot raze null db";
+    DLOG_IF(FATAL, !poisoned_) << "Cannot raze null db";
     return false;
   }
 
   ScopedBusyTimeout busy_timeout(db_);
   busy_timeout.SetTimeout(timeout);
   return Raze();
 }
 
+bool Connection::RazeAndClose() {
+  if (!db_) {
+    DLOG_IF(FATAL, !poisoned_) << "Cannot raze null db";
+    return false;
+  }
+
+  // Raze() cannot run in a transaction.
+  while (transaction_nesting_) {

[pkotwicz, 2013/01/31 19:22:00]

Nit: The braces are not necessary

[Scott Hess - ex-Googler, 2013/02/05 01:20:25]

I'll plead the personal-preference exemption in the style guide.  After years of
seeing it, I can survive without optional braces in an if() statement, but for
loops I'm not sure I agree with the optional part.

+    RollbackTransaction();
+  }
+
+  bool result = Raze();
+
+  CloseInternal(true);
+
+  // Mark the database so that future API calls fail appropriately,
+  // but don't DCHECK (because after calling this function they are
+  // expected to fail).
+  poisoned_ = true;
+
+  return result;
+}
+
 bool Connection::BeginTransaction() {
   if (needs_rollback_) {
     DCHECK_GT(transaction_nesting_, 0);
 
     // When we're going to rollback, fail on this begin and don't actually
     // mark us as entering the nested transaction.
     return false;
   }
 
   bool success = true;
   if (!transaction_nesting_) {
     needs_rollback_ = false;
 
     Statement begin(GetCachedStatement(SQL_FROM_HERE, "BEGIN TRANSACTION"));
     if (!begin.Run())
       return false;
   }
   transaction_nesting_++;
   return success;
 }
 
 void Connection::RollbackTransaction() {
   if (!transaction_nesting_) {
-    DLOG(FATAL) << "Rolling back a nonexistent transaction";
+    DLOG_IF(FATAL, !poisoned_) << "Rolling back a nonexistent transaction";
     return;
   }
 
   transaction_nesting_--;
 
   if (transaction_nesting_ > 0) {
     // Mark the outermost transaction as needing rollback.
     needs_rollback_ = true;
     return;
   }
 
   DoRollback();
 }
 
 bool Connection::CommitTransaction() {
   if (!transaction_nesting_) {
-    DLOG(FATAL) << "Rolling back a nonexistent transaction";
+    DLOG_IF(FATAL, !poisoned_) << "Rolling back a nonexistent transaction";
     return false;
   }
   transaction_nesting_--;
 
   if (transaction_nesting_ > 0) {
     // Mark any nested transactions as failing after we've already got one.
     return !needs_rollback_;
   }
 
   if (needs_rollback_) {
     DoRollback();
     return false;
   }
 
   Statement commit(GetCachedStatement(SQL_FROM_HERE, "COMMIT"));
   return commit.Run();
 }
 
 int Connection::ExecuteAndReturnErrorCode(const char* sql) {
   AssertIOAllowed();
   if (!db_)
     return false;

[pkotwicz, 2013/01/31 19:22:00]

false == SQLITE_OK ?

[Scott Hess - ex-Googler, 2013/02/05 01:20:25]

GOOD CATCH!  I hope that nobody is currently hitting this case and depending on
that, because I'm going with SQLITE_ERROR.  I'll also put in a DLOG_IF(), as
this seems like something callers shouldn't be doing in the first place.

I've also modified Execute() to check for NULL db_, because it seems like at
this point all we know is that the caller is really confused, so I don't trust
the error handler.

   return sqlite3_exec(db_, sql, NULL, NULL, NULL);
 }
 
 bool Connection::Execute(const char* sql) {
   int error = ExecuteAndReturnErrorCode(sql);
   if (error != SQLITE_OK)
     error = OnSqliteError(error, NULL);
 
   // This needs to be a FATAL log because the error case of arriving here is
   // that there's a malformed SQL statement. This can arise in development if
@@ skipping 66 matching lines @@
   if (rc != SQLITE_OK) {
     // This is evidence of a syntax error in the incoming SQL.
     DLOG(FATAL) << "SQL compile error " << GetErrorMessage();
     return new StatementRef();
   }
   return new StatementRef(stmt);
 }
 
 bool Connection::IsSQLValid(const char* sql) {
   AssertIOAllowed();
+  if (!db_) {
+    DLOG_IF(FATAL, !poisoned_) << "Illegal use of connection without a db";
+    return false;
+  }
+
   sqlite3_stmt* stmt = NULL;
   if (sqlite3_prepare_v2(db_, sql, -1, &stmt, NULL) != SQLITE_OK)
     return false;
 
   sqlite3_finalize(stmt);
   return true;
 }
 
 bool Connection::DoesTableExist(const char* table_name) const {
   return DoesTableOrIndexExist(table_name, "table");
 }
 
 bool Connection::DoesIndexExist(const char* index_name) const {
   return DoesTableOrIndexExist(index_name, "index");
 }
 
 bool Connection::DoesTableOrIndexExist(

[pkotwicz, 2013/01/31 19:22:00]

Comment: Calling this method after the connection is poisoned will crash on a
debug build? Is this the expected behavior for this CL (irrespective of what's
the correct behavior long term)?

[Scott Hess - ex-Googler, 2013/02/05 01:20:25]

I don't think so - it should fail comprehensively and then return false.  I'll
check that that holds.  Do you think it's worthwhile to unit test this?  Hmm, I
guess that answers itself.

LATER: Of course - I was wrong, you're completely right.  I need to think on
this issue, maybe/probably I just need to refactor StatementRef() construction a
bit, to be less implicit.

EVEN LATER: Man, I'm dying, here.  Multiple Statement instances referencing the
same StatementRef is just wrong, I can see why we might want a statement cache,
but maybe ownership of the sqlite3_stmt could just be shifted directly to the
Statement instance and back in the destructor.  Sigh.

     const char* name, const char* type) const {
   const char* kSql = "SELECT name FROM sqlite_master WHERE type=? AND name=?";
   Statement statement(GetUntrackedStatement(kSql));
   statement.BindString(0, type);
   statement.BindString(1, name);
 
   return statement.Step();  // Table exists if any row was returned.
 }
 
 bool Connection::DoesColumnExist(const char* table_name,
                                  const char* column_name) const {
   std::string sql("PRAGMA TABLE_INFO(");
   sql.append(table_name);
   sql.append(")");
 
   Statement statement(GetUntrackedStatement(sql.c_str()));
   while (statement.Step()) {
     if (!statement.ColumnString(1).compare(column_name))
       return true;
   }
   return false;
 }
 
 int64 Connection::GetLastInsertRowId() const {
   if (!db_) {
-    DLOG(FATAL) << "Illegal use of connection without a db";
+    DLOG_IF(FATAL, !poisoned_) << "Illegal use of connection without a db";
     return 0;
   }
   return sqlite3_last_insert_rowid(db_);
 }
 
 int Connection::GetLastChangeCount() const {
   if (!db_) {
-    DLOG(FATAL) << "Illegal use of connection without a db";
+    DLOG_IF(FATAL, !poisoned_) << "Illegal use of connection without a db";
     return 0;
   }
   return sqlite3_changes(db_);
 }
 
 int Connection::GetErrorCode() const {
   if (!db_)
     return SQLITE_ERROR;
   return sqlite3_errcode(db_);
 }
@@ skipping 11 matching lines @@
 
 const char* Connection::GetErrorMessage() const {
   if (!db_)
     return "sql::Connection has no connection.";
   return sqlite3_errmsg(db_);
 }
 
 bool Connection::OpenInternal(const std::string& file_name) {
   AssertIOAllowed();
 
+  // Reset, in case connection is re-used.  This usage may not be an
+  // intentional API contract, but tests do it.
+  poisoned_ = false;
+
   if (db_) {
     DLOG(FATAL) << "sql::Connection is already open.";
     return false;
   }
 
   int err = sqlite3_open(file_name.c_str(), &db_);
   if (err != SQLITE_OK) {
     // Histogram failures specific to initial open for debugging
     // purposes.
     UMA_HISTOGRAM_ENUMERATION("Sqlite.OpenFailure", err & 0xff, 50);
@@ skipping 89 matching lines @@
 }
 
 void Connection::StatementRefDeleted(StatementRef* ref) {
   StatementRefSet::iterator i = open_statements_.find(ref);
   if (i == open_statements_.end())
     DLOG(FATAL) << "Could not find statement";
   else
     open_statements_.erase(i);
 }
 
-void Connection::ClearCache() {
-  statement_cache_.clear();
-
-  // The cache clear will get most statements. There may be still be references
-  // to some statements that are held by others (including one-shot statements).
-  // This will deactivate them so they can't be used again.
-  for (StatementRefSet::iterator i = open_statements_.begin();
-       i != open_statements_.end(); ++i)
-    (*i)->Close();
-}
-
 int Connection::OnSqliteError(int err, sql::Statement *stmt) {
   // Strip extended error codes.
   int base_err = err&0xff;
 
   static size_t kSqliteErrorMax = 50;
   UMA_HISTOGRAM_ENUMERATION("Sqlite.Error", base_err, kSqliteErrorMax);
   if (!error_histogram_name_.empty()) {
     // TODO(shess): The histogram macros create a bit of static
     // storage for caching the histogram object.  Since SQLite is
     // being used for I/O, generally without error, this code
@@ skipping 15 matching lines @@
 
   if (error_delegate_.get())
     return error_delegate_->OnError(err, this, stmt);
 
   // The default handling is to assert on debug and to ignore on release.
   DLOG(FATAL) << GetErrorMessage();
   return err;
 }
 
 }  // namespace sql