commit 22fba89b98c218f81bfeeedf29e38dc3afba6734
parent 101f68be6793513e9f868e75640461f5cdd4dcf9
Author: JohnMcPMS <johnmcp@microsoft.com>
Date: Mon, 26 Feb 2024 10:02:54 -0800
Make SQLite wrapper more resilient (#4196)
We are seeing some errors from the SQLite usage that appear to largely
be related to contention and the `FAIL_FAST` behavior for savepoint
errors. This change is attempting to address the issue by making
contention less impactful and moving away from `FAIL_FAST`.
## Change
- Adds a busy timeout to SQLite connections by default and provides a
method to change the value if callers desire. This should prevent small
contentions from causing errors.
- Moves from using `FAIL_FAST` on critical errors to simply "closing"
the connection, making it non-functional for standard use.
- No longer requests this failure behavior for savepoint commit errors;
the rollback can handle that if needed.
Diffstat:
6 files changed, 198 insertions(+), 32 deletions(-)
diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw
@@ -2804,14 +2804,17 @@ Please specify one of them using the --source option to proceed.</value>
<data name="RepairOperationNotSupported" xml:space="preserve">
<value>The installer technology in use does not support repair.</value>
</data>
- <data name="NoAdminRepairForUserScopePackage" xml:space="preserve">
+ <data name="NoAdminRepairForUserScopePackage" xml:space="preserve">
<value>The package installed for user scope cannot be repaired when running with administrator privileges.</value>
</data>
<data name="APPINSTALLER_CLI_ERROR_ADMIN_CONTEXT_REPAIR_PROHIBITED" xml:space="preserve">
<value>Repair operations involving administrator privileges are not permitted on packages installed within the user scope.</value>
- </data>
+ </data>
<data name="RepairFailedWithCode" xml:space="preserve">
<value>Repair failed with exit code: {0}</value>
<comment>{Locked="{0}"} Error message displayed when an attempt to repair an application package fails. {0} is a placeholder replaced by an error code.</comment>
</data>
+ <data name="APPINSTALLER_CLI_ERROR_SQLITE_CONNECTION_TERMINATED" xml:space="preserve">
+ <value>The SQLite connection was terminated to prevent corruption.</value>
+ </data>
</root>
\ No newline at end of file
diff --git a/src/AppInstallerCLITests/SQLiteWrapper.cpp b/src/AppInstallerCLITests/SQLiteWrapper.cpp
@@ -312,6 +312,90 @@ TEST_CASE("SQLiteWrapper_PrepareFailure", "[sqlitewrapper]")
REQUIRE_THROWS_HR(builder.Prepare(connection), MAKE_HRESULT(SEVERITY_ERROR, FACILITY_SQLITE, SQLITE_ERROR));
}
+TEST_CASE("SQLiteWrapper_BusyTimeout_None", "[sqlitewrapper]")
+{
+ TestCommon::TempFile tempFile{ "repolibtest_tempdb"s, ".db"s };
+ INFO("Using temporary file named: " << tempFile.GetPath());
+
+ wil::unique_event busy, done;
+ busy.create();
+ done.create();
+
+ std::thread busyThread([&]()
+ {
+ Connection threadConnection = Connection::Create(tempFile, Connection::OpenDisposition::Create);
+ Statement threadStatement = Statement::Create(threadConnection, "BEGIN EXCLUSIVE TRANSACTION");
+ threadStatement.Execute();
+ busy.SetEvent();
+ done.wait(500);
+ });
+ busyThread.detach();
+
+ busy.wait(500);
+
+ Connection testConnection = Connection::Create(tempFile, Connection::OpenDisposition::ReadWrite);
+ testConnection.SetBusyTimeout(0ms);
+ Statement testStatement = Statement::Create(testConnection, "BEGIN EXCLUSIVE TRANSACTION");
+ REQUIRE_THROWS_HR(testStatement.Execute(), MAKE_HRESULT(SEVERITY_ERROR, FACILITY_SQLITE, SQLITE_BUSY));
+
+ done.SetEvent();
+}
+
+TEST_CASE("SQLiteWrapper_BusyTimeout_Some", "[sqlitewrapper]")
+{
+ TestCommon::TempFile tempFile{ "repolibtest_tempdb"s, ".db"s };
+ INFO("Using temporary file named: " << tempFile.GetPath());
+
+ wil::unique_event busy, ready, done;
+ busy.create();
+ ready.create();
+ done.create();
+
+ std::thread busyThread([&]()
+ {
+ Connection threadConnection = Connection::Create(tempFile, Connection::OpenDisposition::Create);
+ Statement threadBeginStatement = Statement::Create(threadConnection, "BEGIN EXCLUSIVE TRANSACTION");
+ Statement threadCommitStatement = Statement::Create(threadConnection, "COMMIT");
+ threadBeginStatement.Execute();
+ busy.SetEvent();
+ ready.wait(500);
+ done.wait(100);
+ threadCommitStatement.Execute();
+ });
+ busyThread.detach();
+
+ busy.wait(500);
+
+ Connection testConnection = Connection::Create(tempFile, Connection::OpenDisposition::ReadWrite);
+ testConnection.SetBusyTimeout(500ms);
+ Statement testStatement = Statement::Create(testConnection, "BEGIN EXCLUSIVE TRANSACTION");
+ ready.SetEvent();
+ testStatement.Execute();
+
+ done.SetEvent();
+}
+
+TEST_CASE("SQLiteWrapper_CloseConnectionOnError", "[sqlitewrapper]")
+{
+ Connection connection = Connection::Create(SQLITE_MEMORY_DB_CONNECTION_TARGET, Connection::OpenDisposition::Create);
+
+ Builder::StatementBuilder builder;
+ builder.CreateTable(s_tableName).Columns({
+ Builder::ColumnBuilder(s_firstColumn, Builder::Type::Int),
+ Builder::ColumnBuilder(s_secondColumn, Builder::Type::Text),
+ });
+
+ Statement createTable = builder.Prepare(connection);
+ REQUIRE_FALSE(createTable.Step());
+ REQUIRE(createTable.GetState() == Statement::State::Completed);
+
+ createTable.Reset();
+ REQUIRE_THROWS(createTable.Step(true));
+
+ // Do anything that needs the connection
+ REQUIRE_THROWS_HR(connection.GetLastInsertRowID(), APPINSTALLER_CLI_ERROR_SQLITE_CONNECTION_TERMINATED);
+}
+
TEST_CASE("SQLBuilder_SimpleSelectBind", "[sqlbuilder]")
{
Connection connection = Connection::Create(SQLITE_MEMORY_DB_CONNECTION_TARGET, Connection::OpenDisposition::Create);
diff --git a/src/AppInstallerSharedLib/Errors.cpp b/src/AppInstallerSharedLib/Errors.cpp
@@ -213,6 +213,7 @@ namespace AppInstaller
WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_EXEC_REPAIR_FAILED, "Repair operation failed."),
WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_REPAIR_NOT_SUPPORTED, "The installer technology in use doesn't support repair."),
WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_ADMIN_CONTEXT_REPAIR_PROHIBITED, "Repair operations involving administrator privileges are not permitted on packages installed within the user scope."),
+ WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_SQLITE_CONNECTION_TERMINATED, "The SQLite connection was terminated to prevent corruption."),
// Install errors.
WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_INSTALL_PACKAGE_IN_USE, "Application is currently running. Exit the application then try again."),
diff --git a/src/AppInstallerSharedLib/Public/AppInstallerErrors.h b/src/AppInstallerSharedLib/Public/AppInstallerErrors.h
@@ -143,6 +143,7 @@
#define APPINSTALLER_CLI_ERROR_EXEC_REPAIR_FAILED ((HRESULT)0x8A15007B)
#define APPINSTALLER_CLI_ERROR_REPAIR_NOT_SUPPORTED ((HRESULT)0x8A15007C)
#define APPINSTALLER_CLI_ERROR_ADMIN_CONTEXT_REPAIR_PROHIBITED ((HRESULT)0x8A15007D)
+#define APPINSTALLER_CLI_ERROR_SQLITE_CONNECTION_TERMINATED ((HRESULT)0x8A15007E)
// Install errors.
#define APPINSTALLER_CLI_ERROR_INSTALL_PACKAGE_IN_USE ((HRESULT)0x8A150101)
diff --git a/src/AppInstallerSharedLib/Public/winget/SQLiteWrapper.h b/src/AppInstallerSharedLib/Public/winget/SQLiteWrapper.h
@@ -8,6 +8,7 @@
#include <AppInstallerLogging.h>
#include <AppInstallerLanguageUtilities.h>
+#include <memory>
#include <string>
#include <string_view>
#include <tuple>
@@ -125,6 +126,23 @@ namespace AppInstaller::SQLite
template <typename T>
using ParameterSpecifics = ParameterSpecificsImpl<std::decay_t<T>>;
+
+ // Allows the connection to be shared so that it can be closed in some circumstances.
+ struct SharedConnection
+ {
+ // Disables the connection, causing an exception to be thrown by `get`.
+ void Disable();
+
+ // Gets the connection object if active.
+ sqlite3* Get() const;
+
+ // Gets the connection object for creation.
+ sqlite3** GetPtr();
+
+ private:
+ std::atomic_bool m_active = true;
+ wil::unique_any<sqlite3*, decltype(sqlite3_close_v2), sqlite3_close_v2> m_dbconn;
+ };
}
// A SQLite exception.
@@ -133,9 +151,13 @@ namespace AppInstaller::SQLite
SQLiteException(int error) : wil::ResultException(MAKE_HRESULT(SEVERITY_ERROR, FACILITY_SQLITE, error)) {}
};
+ struct Statement;
+
// The connection to a database.
struct Connection
{
+ friend Statement;
+
// The disposition for opening a database connection.
enum class OpenDisposition : int
{
@@ -180,13 +202,20 @@ namespace AppInstaller::SQLite
//. Gets the (fixed but arbitrary) identifier for this connection.
size_t GetID() const;
- operator sqlite3* () const { return m_dbconn.get(); }
+ // Sets the busy timeout for the connection.
+ void SetBusyTimeout(std::chrono::milliseconds timeout);
+
+ operator sqlite3* () const { return m_dbconn->Get(); }
+
+ protected:
+ // Gets the shared connection.
+ std::shared_ptr<details::SharedConnection> GetSharedConnection() const;
private:
Connection(const std::string& target, OpenDisposition disposition, OpenFlags flags);
size_t m_id = 0;
- wil::unique_any<sqlite3*, decltype(sqlite3_close_v2), sqlite3_close_v2> m_dbconn;
+ std::shared_ptr<details::SharedConnection> m_dbconn;
};
// A SQL statement.
@@ -234,10 +263,10 @@ namespace AppInstaller::SQLite
// Evaluate the statement; either retrieving the next row or executing some action.
// Returns true if there is a row of data, or false if there is none.
// This return value is the equivalent of 'GetState() == State::HasRow' after calling Step.
- bool Step(bool failFastOnError = false);
+ bool Step(bool closeConnectionOnError = false);
// Equivalent to Step, but does not ever expect a result, throwing if one is retrieved.
- void Execute(bool failFastOnError = false);
+ void Execute(bool closeConnectionOnError = false);
// Gets a boolean value that indicates whether the specified column value is null in the current row.
// The index is 0 based.
@@ -282,6 +311,7 @@ namespace AppInstaller::SQLite
return std::make_tuple(details::ParameterSpecifics<Values>::GetColumn(m_stmt.get(), I)...);
}
+ std::shared_ptr<details::SharedConnection> m_dbconn;
size_t m_connectionId = 0;
size_t m_id = 0;
wil::unique_any<sqlite3_stmt*, decltype(sqlite3_finalize), sqlite3_finalize> m_stmt;
@@ -303,7 +333,7 @@ namespace AppInstaller::SQLite
~Savepoint();
// Rolls back the Savepoint.
- void Rollback();
+ void Rollback(bool throwOnError = true);
// Commits the Savepoint.
void Commit();
diff --git a/src/AppInstallerSharedLib/SQLiteWrapper.cpp b/src/AppInstallerSharedLib/SQLiteWrapper.cpp
@@ -7,6 +7,7 @@
#include <wil/result_macros.h>
+using namespace std::chrono_literals;
using namespace std::string_view_literals;
// Enable this to have all Statement constructions output the associated query plan.
@@ -16,11 +17,14 @@ using namespace std::string_view_literals;
#include <stack>
#endif
+// Connection is used twice
+#define SQLITE_ERROR_MSG(_error_,_connection_) (_connection_ ? sqlite3_errmsg(_connection_) : sqlite3_errstr(_error_))
+
#define THROW_SQLITE(_error_,_connection_) \
do { \
int _ts_sqliteReturnValue = (_error_); \
sqlite3* _ts_sqliteConnection = (_connection_); \
- THROW_EXCEPTION_MSG(SQLiteException(_ts_sqliteReturnValue), "%hs", _ts_sqliteConnection ? sqlite3_errmsg(_ts_sqliteConnection) : sqlite3_errstr(_ts_sqliteReturnValue)); \
+ THROW_EXCEPTION_MSG(SQLiteException(_ts_sqliteReturnValue), "%hs", SQLITE_ERROR_MSG(_ts_sqliteReturnValue, _ts_sqliteConnection)); \
} while (0,0)
#define THROW_IF_SQLITE_FAILED(_statement_,_connection_) \
@@ -144,22 +148,40 @@ namespace AppInstaller::SQLite
return {};
}
}
+
+ void SharedConnection::Disable()
+ {
+ m_active = false;
+ }
+
+ sqlite3* SharedConnection::Get() const
+ {
+ THROW_HR_IF(APPINSTALLER_CLI_ERROR_SQLITE_CONNECTION_TERMINATED, !m_active.load());
+ return m_dbconn.get();
+ }
+
+ sqlite3** SharedConnection::GetPtr()
+ {
+ return &m_dbconn;
+ }
}
Connection::Connection(const std::string& target, OpenDisposition disposition, OpenFlags flags)
{
+ m_dbconn = std::make_shared<details::SharedConnection>();
m_id = GetNextConnectionId();
AICLI_LOG(SQL, Info, << "Opening SQLite connection #" << m_id << ": '" << target << "' [" << std::hex << static_cast<int>(disposition) << ", " << std::hex << static_cast<int>(flags) << "]");
// Always force connection serialization until we determine that there are situations where it is not needed
int resultingFlags = static_cast<int>(disposition) | static_cast<int>(flags) | SQLITE_OPEN_FULLMUTEX;
- THROW_IF_SQLITE_FAILED(sqlite3_open_v2(target.c_str(), &m_dbconn, resultingFlags, nullptr), nullptr);
+ THROW_IF_SQLITE_FAILED(sqlite3_open_v2(target.c_str(), m_dbconn->GetPtr(), resultingFlags, nullptr), nullptr);
}
Connection Connection::Create(const std::string& target, OpenDisposition disposition, OpenFlags flags)
{
Connection result{ target, disposition, flags };
-
- THROW_IF_SQLITE_FAILED(sqlite3_extended_result_codes(result.m_dbconn.get(), 1), result.m_dbconn.get());
+
+ THROW_IF_SQLITE_FAILED(sqlite3_extended_result_codes(result.m_dbconn->Get(), 1), result.m_dbconn->Get());
+ result.SetBusyTimeout(250ms);
return result;
}
@@ -167,17 +189,17 @@ namespace AppInstaller::SQLite
void Connection::EnableICU()
{
AICLI_LOG(SQL, Verbose, << "Enabling ICU");
- THROW_IF_SQLITE_FAILED(sqlite3IcuInit(m_dbconn.get()), m_dbconn.get());
+ THROW_IF_SQLITE_FAILED(sqlite3IcuInit(m_dbconn->Get()), m_dbconn->Get());
}
rowid_t Connection::GetLastInsertRowID()
{
- return sqlite3_last_insert_rowid(m_dbconn.get());
+ return sqlite3_last_insert_rowid(m_dbconn->Get());
}
int Connection::GetChanges() const
{
- return sqlite3_changes(m_dbconn.get());
+ return sqlite3_changes(m_dbconn->Get());
}
size_t Connection::GetID() const
@@ -185,8 +207,19 @@ namespace AppInstaller::SQLite
return m_id;
}
+ void Connection::SetBusyTimeout(std::chrono::milliseconds timeout)
+ {
+ THROW_IF_SQLITE_FAILED(sqlite3_busy_timeout(m_dbconn->Get(), static_cast<int>(timeout.count())), m_dbconn->Get());
+ }
+
+ std::shared_ptr<details::SharedConnection> Connection::GetSharedConnection() const
+ {
+ return m_dbconn;
+ }
+
Statement::Statement(const Connection& connection, std::string_view sql)
{
+ m_dbconn = connection.GetSharedConnection();
m_connectionId = connection.GetID();
m_id = GetNextStatementId();
AICLI_LOG(SQL, Verbose, << "Preparing statement #" << m_connectionId << '-' << m_id << ": " << sql);
@@ -253,7 +286,7 @@ namespace AppInstaller::SQLite
return { connection, sql };
}
- bool Statement::Step(bool failFastOnError)
+ bool Statement::Step(bool closeConnectionOnError)
{
AICLI_LOG(SQL, Verbose, << "Stepping statement #" << m_connectionId << '-' << m_id);
int result = sqlite3_step(m_stmt.get());
@@ -273,20 +306,19 @@ namespace AppInstaller::SQLite
else
{
m_state = State::Error;
- if (failFastOnError)
- {
- FAIL_FAST_MSG("Critical SQL statement failed");
- }
- else
+
+ if (closeConnectionOnError)
{
- THROW_SQLITE(result, sqlite3_db_handle(m_stmt.get()));
+ m_dbconn->Disable();
}
+
+ THROW_SQLITE(result, sqlite3_db_handle(m_stmt.get()));
}
}
- void Statement::Execute(bool failFastOnError)
+ void Statement::Execute(bool closeConnectionOnError)
{
- THROW_HR_IF(E_UNEXPECTED, Step(failFastOnError));
+ THROW_HR_IF(E_UNEXPECTED, Step(closeConnectionOnError));
}
bool Statement::GetColumnIsNull(int column)
@@ -323,20 +355,35 @@ namespace AppInstaller::SQLite
Savepoint::~Savepoint()
{
- Rollback();
+ // Prevent a termination by not throwing on errors here
+ Rollback(false);
}
- void Savepoint::Rollback()
+ void Savepoint::Rollback(bool throwOnError)
{
if (m_inProgress)
{
- AICLI_LOG(SQL, Verbose, << "Roll back savepoint: " << m_name);
- m_rollbackTo.Step(true);
- // 'ROLLBACK TO' *DOES NOT* remove the savepoint from the transaction stack.
- // In order to remove it, we must RELEASE. Since we just invoked a ROLLBACK TO
- // this should have the effect of 'committing' nothing.
- m_release.Step(true);
+ // Only try rollback once
m_inProgress = false;
+
+ try
+ {
+ AICLI_LOG(SQL, Verbose, << "Roll back savepoint: " << m_name);
+ m_rollbackTo.Step(true);
+ // 'ROLLBACK TO' *DOES NOT* remove the savepoint from the transaction stack.
+ // In order to remove it, we must RELEASE. Since we just invoked a ROLLBACK TO
+ // this should have the effect of 'committing' nothing.
+ m_release.Step(true);
+ }
+ catch (...)
+ {
+ if (throwOnError)
+ {
+ throw;
+ }
+
+ LOG_CAUGHT_EXCEPTION();
+ }
}
}
@@ -345,7 +392,7 @@ namespace AppInstaller::SQLite
if (m_inProgress)
{
AICLI_LOG(SQL, Verbose, << "Commit savepoint: " << m_name);
- m_release.Step(true);
+ m_release.Step();
m_inProgress = false;
}
}
