Bug 34152 – SQLiteTransactions's destructor should not try to rollback a transaction in progress

• Component: New Bugs
• Status: RESOLVED FIXED

[Dumitru Daniliuc]

Currently, SQLiteTransaction's destructor tries to rollback a transaction that's in progress. This is not the right thing to do and leads to an assertion failure if the SQLiteTransaction object happens to be destroyed on the main thread (this could happen at shutdown). I believe the right thing to do is to do nothing in ~SQLiteTransaction. SQLTransaction already has logic to rollback a transaction if a transaction's error callback is invoked; and if a shutdown happens in the middle of a transaction, the DB thread makes sure to close all DB connections, which makes sqlite automatically rollback any transaction in progress, according to http://www.sqlite.org/c3ref/close.html.

[Dumitru Daniliuc]

Created attachment 47383 [details]
patch

[Michael Nordman]

Hmmm... I don't think this is a good change.

sql/SQLiteTransaction dtor with the call to rollback() if in progress looks very correct. 

It sounds like there are some cases where that behavior doesn't work well for storage/SQLTransaction, but I think the place to 'fix' whatever issues those are would be closer to storage/SQLTransaction.

[Eric Seidel (no email)]

Comment on attachment 47383 [details]
patch

Should we be replacing this with an ASSERT?  r- pending response to michael's comment.

[Dumitru Daniliuc]

Created attachment 47488 [details]
patch

An alternative approach: destroy the SQLiteTransaction objects associated with in-progress transactions (and thus, roll back those transactions) on the DB thread, before the DB thread goes away, but after it stops processing tasks from its task queue.

[Michael Nordman]

(In reply to comment #4)
> Created an attachment (id=47488) [details]
> patch

This approach looks much better to me.

It may nice to ASSERT(!m_sqliteTransaction) in the ~SQLTransation.

442	    m_database->m_databaseAuthorizer->disable();
443	    m_sqliteTransaction->commit();
444	    m_database->m_databaseAuthorizer->enable();
445	
446	    // If the commit failed, the transaction will still be marked as "in progress"
447	    if (m_sqliteTransaction->inProgress()) {
448	        m_transactionError = SQLError::create(0, "failed to commit the transaction");
449	        handleTransactionError(false);
450	        return;
451	    }

           m_sqliteTransaction.clear();

I think we could make that assertion if the ptr was cleared after successfully committing in postflightAndCommit(). The error cases and early thread shutdown cases result in a cleared ptr, only the success case leaves it allocated.

[Dumitru Daniliuc]

Created attachment 47560 [details]
patch

Please take another look.

[Michael Nordman]

this lgtm

[Michael Nordman]

Somewhat unrelated to this change...

void SQLiteTransaction::commit()
{
    if (m_inProgress) {
        ASSERT(m_db.m_transactionInProgress);
        m_db.executeCommand("COMMIT;");
        m_inProgress = false;
        m_db.m_transactionInProgress = false;
    }
}

... what if the COMMIT fails?

[Dumitru Daniliuc]

(In reply to comment #8)
> Somewhat unrelated to this change...
> 
> void SQLiteTransaction::commit()
> {
>     if (m_inProgress) {
>         ASSERT(m_db.m_transactionInProgress);
>         m_db.executeCommand("COMMIT;");
>         m_inProgress = false;
>         m_db.m_transactionInProgress = false;
>     }
> }
> 
> ... what if the COMMIT fails?

This is a bug that should be fixed in this patch, because SQLTransaction is expecting m_inProgress to be false if commit() fails. SQLTransaction treats a failed commit() as any other transaction error, and tries to rollback the transaction.

New patch coming in a minute.

[Dumitru Daniliuc]

Actually, while trying to make the last change Michael suggested ("what if COMMIT fails?"), we ran into a much bigger problem, that is way beyond the scope of this bug. So Michael and I decided that the last patch that I uploaded (and Michael LGTM'd) is the most appropriate patch for this bug.

Eric (or whoever will be reviewing this patch), can you please take a look at the last patch, and ignore Michael's comments about COMMIT failing for now? I will open a separate bug for that issue.

[Dumitru Daniliuc]

Created attachment 47743 [details]
patch

[Eric Seidel (no email)]

Comment on attachment 47743 [details]
patch

OK.

[Dumitru Daniliuc]

Landed as r54162.