app/sql/connection hygiene enforcement.
For awhile we had an error where an invalid page_size was being passed in, and SQLite silently ignored it. No more! Fix cache_size logging line. Pull the exclusive locking up to fail more obviously in case someone else has the exclusive lock.
Also, I don't think our code is likely handling the SQLITE_BUSY case correctly. Added some initial code to execute some sql with a timeout.
BUG=56559
TEST=unit tests
Review URL: http://codereview.chromium.org/3413028
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@60799 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/app/sql/connection.cc b/app/sql/connection.cc
index d227468d..b52655a 100644
--- a/app/sql/connection.cc
+++ b/app/sql/connection.cc
@@ -13,6 +13,34 @@
#include "base/utf_string_conversions.h"
#include "third_party/sqlite/sqlite3.h"
+namespace {
+
+// Spin for up to a second waiting for the lock to clear when setting
+// up the database.
+// TODO(shess): Better story on this. http://crbug.com/56559
+const base::TimeDelta kBusyTimeout = base::TimeDelta::FromSeconds(1);
+
+class ScopedBusyTimeout {
+ public:
+ explicit ScopedBusyTimeout(sqlite3* db)
+ : db_(db) {
+ }
+ ~ScopedBusyTimeout() {
+ sqlite3_busy_timeout(db_, 0);
+ }
+
+ int SetTimeout(base::TimeDelta timeout) {
+ DCHECK_LT(timeout.InMilliseconds(), INT_MAX);
+ return sqlite3_busy_timeout(db_,
+ static_cast<int>(timeout.InMilliseconds()));
+ }
+
+ private:
+ sqlite3* db_;
+};
+
+} // namespace
+
namespace sql {
bool StatementID::operator<(const StatementID& other) const {
@@ -166,6 +194,15 @@
return sqlite3_exec(db_, sql, NULL, NULL, NULL) == SQLITE_OK;
}
+bool Connection::ExecuteWithTimeout(const char* sql, base::TimeDelta timeout) {
+ if (!db_)
+ return false;
+
+ ScopedBusyTimeout busy_timeout(db_);
+ busy_timeout.SetTimeout(timeout);
+ return sqlite3_exec(db_, sql, NULL, NULL, NULL) == SQLITE_OK;
+}
+
bool Connection::HasCachedStatement(const StatementID& id) const {
return statement_cache_.find(id) != statement_cache_.end();
}
@@ -296,19 +333,36 @@
err = sqlite3_extended_result_codes(db_, 1);
DCHECK_EQ(err, SQLITE_OK) << "Could not enable extended result codes";
+ // If indicated, lock up the database before doing anything else, so
+ // that the following code doesn't have to deal with locking.
+ // TODO(shess): This code is brittle. Find the cases where code
+ // doesn't request |exclusive_locking_| and audit that it does the
+ // right thing with SQLITE_BUSY, and that it doesn't make
+ // assumptions about who might change things in the database.
+ // http://crbug.com/56559
+ if (exclusive_locking_) {
+ // TODO(shess): This should probably be a full CHECK(). Code
+ // which requests exclusive locking but doesn't get it is almost
+ // certain to be ill-tested.
+ if (!Execute("PRAGMA locking_mode=EXCLUSIVE"))
+ NOTREACHED() << "Could not set locking mode: " << GetErrorMessage();
+ }
+
if (page_size_ != 0) {
- if (!Execute(StringPrintf("PRAGMA page_size=%d", page_size_).c_str()))
- NOTREACHED() << "Could not set page size";
+ // Enforce SQLite restrictions on |page_size_|.
+ DCHECK(!(page_size_ & (page_size_ - 1)))
+ << " page_size_ " << page_size_ << " is not a power of two.";
+ static const int kSqliteMaxPageSize = 32768; // from sqliteLimit.h
+ DCHECK_LE(page_size_, kSqliteMaxPageSize);
+ const std::string sql = StringPrintf("PRAGMA page_size=%d", page_size_);
+ if (!ExecuteWithTimeout(sql.c_str(), kBusyTimeout))
+ NOTREACHED() << "Could not set page size: " << GetErrorMessage();
}
if (cache_size_ != 0) {
- if (!Execute(StringPrintf("PRAGMA cache_size=%d", cache_size_).c_str()))
- NOTREACHED() << "Could not set page size";
- }
-
- if (exclusive_locking_) {
- if (!Execute("PRAGMA locking_mode=EXCLUSIVE"))
- NOTREACHED() << "Could not set locking mode.";
+ const std::string sql = StringPrintf("PRAGMA cache_size=%d", cache_size_);
+ if (!ExecuteWithTimeout(sql.c_str(), kBusyTimeout))
+ NOTREACHED() << "Could not set cache size: " << GetErrorMessage();
}
return true;
