Implement sql::Connection::RazeAndClose().

sql/connection.cc

Index: sql/connection.cc
diff --git a/sql/connection.cc b/sql/connection.cc
index aefac60fac7e3fa37b75611eebf3dba457e48204..1a795b20525fd86c74ea6f9b383176bfc4f93936 100644
--- a/sql/connection.cc
+++ b/sql/connection.cc
@@ -76,28 +76,31 @@ 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
@@ -111,6 +114,11 @@ void Connection::StatementRef::Close() {
     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()
@@ -141,22 +149,28 @@ bool Connection::OpenInMemory() {
   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
@@ -172,11 +186,19 @@ void Connection::Close() {
   }
 }
 
+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;
   }
 
@@ -202,7 +224,7 @@ bool Connection::Raze() {
   AssertIOAllowed();
 
   if (!db_) {
-    DLOG(FATAL) << "Cannot raze null db";
+    DLOG_IF(FATAL, !poisoned_) << "Cannot raze null db";
     return false;
   }
 
@@ -292,7 +314,7 @@ bool Connection::Raze() {
 
 bool Connection::RazeWithTimout(base::TimeDelta timeout) {
   if (!db_) {
-    DLOG(FATAL) << "Cannot raze null db";
+    DLOG_IF(FATAL, !poisoned_) << "Cannot raze null db";
     return false;
   }
 
@@ -301,6 +323,29 @@ bool Connection::RazeWithTimout(base::TimeDelta 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);
@@ -324,7 +369,7 @@ bool Connection::BeginTransaction() {
 
 void Connection::RollbackTransaction() {
   if (!transaction_nesting_) {
-    DLOG(FATAL) << "Rolling back a nonexistent transaction";
+    DLOG_IF(FATAL, !poisoned_) << "Rolling back a nonexistent transaction";
     return;
   }
 
@@ -341,7 +386,7 @@ void Connection::RollbackTransaction() {
 
 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_--;
@@ -450,6 +495,11 @@ scoped_refptr<Connection::StatementRef> Connection::GetUntrackedStatement(
 
 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;
@@ -492,7 +542,7 @@ bool Connection::DoesColumnExist(const char* table_name,
 
 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_);
@@ -500,7 +550,7 @@ int64 Connection::GetLastInsertRowId() const {
 
 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_);
@@ -532,6 +582,10 @@ const char* Connection::GetErrorMessage() const {
 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;
@@ -641,17 +695,6 @@ void Connection::StatementRefDeleted(StatementRef* ref) {
     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;