Transactions auto-close when queuing microtasks between executeSql() calls

[trajano]

I'm tracing through the code for a bug I opened in Expo and led me to this repo because they have a revendor of your code.

What I am trying to do is avoid doing chained callbacks and use async/await, for me to do that I wrapped some of the WebSQL code so that it uses the promise API to resolve the callbacks.

Almost everything goes well except when I chain two executesql in a single transaction where I await. I am suspecting it may be immediate triggering the execution of the SQL tasks, but when it does it, it also completes the transaction since the queue is empty already.

I've created a snack that shows this. But I am unsure how to fix it.

[trajano]

Looking further into it when using promises alone things are okay. So I narrowed it down even further in that it is due to the async / await. Which "yields" and may be triggering the immediate to run. Just promise chaining will not do that.

[nolanlawson]

Hmm, based on your repro, expo-sqlite is implemented on top of iOS using React Native, and you have a custom implementation of node-websql's SQLite adapter running on top of that. Are you able to repro using only node-websql? If not, could this be a problem in expo-sqlite or in your adapter?

[trajano]

I am suspecting it could be in expo itself too. But Expo's implementation is pretty light in terms of code. However, it is running on top of React Native iOS and Android

[trajano]

I have an mvce https://github.com/trajano/websql-mvce that proves that it does not work when doing the async/await style. I tweaked it around since the @types/websql does not work and I don't have Expo's typings.

When the asyncAwait is used. It runs the first SQL in the transaction but does not execute the other two. Just like in Expo.

[nolanlawson]

I think I might understand after looking at your repro. It sounds like basically you're saying that the code works when you use callbacks, but not async await. In particular, if you try to break up a single transaction into multiple operations across multiple promises (each handled using async/await), then the transaction automatically closes.

This makes sense to me, because the WebSQL API was designed before Promises ever arrived in browsers. It wasn't really designed to work well with Promises.

I guess the closest analog is how IndexedDB eventually allowed microtasks to execute without closing the transaction: https://github.com/w3c/IndexedDB/issues/87. Based on this analog, I guess you could say that node-websql should match what happened with IndexedDB.

Here is the minimal repro of your issue, as I understand it:

const openDatabase = require('websql')
const db = openDatabase(':memory:', '1.0', 'yolo', 100000);

async function main() {
  const txn = await new Promise(resolve => {
    db.transaction(resolve)
  })
  await new Promise(resolve => {
    txn.executeSql('CREATE TABLE foo (bar text);', [], resolve)
  })
  await new Promise(resolve => {
    txn.executeSql('INSERT INTO foo VALUES ("baz")', [], resolve)
  })
  const res = await new Promise(resolve => {
    txn.executeSql('SELECT * FROM foo', [], (txn, res) => resolve(res))
  })
  console.log('res', res)
}

main()

As I understand it, you would expect to be able to chain all these operations in the same transaction, but instead what actually happens is that the transaction closes after the CREATE call.

[trajano]

Yup. I worked around it by creating a new Transaction class that does not use websql transaction semantics. Instead I relied on Expo's SQLite itself, but I think it can be expanded to plain sqlite3

The concept was to use db.exec(...) and use begin transaction commit and rollback. I basically wrapped the exec method which allowed for multiple SQLs to only run one single SQL.

  /**
   * Wraps the exec command but only supports ONE statement without notion of a transaction context.
   * @param sqlStatement SQL statement
   * @param args  arguments array
   * @param readOnly if the exec should be readonly
   * @returns result set.
   */
  async executeSqlAsync(
    sqlStatement: string,
    args: any[] = [],
    readOnly = false
  ): Promise<ResultSet> {
    return new Promise<ResultSet>((resolve, reject) => {
      this.db.exec(
        [{ sql: sqlStatement, args }],
        readOnly,
        (error, resultSet) => {
          if (error) {
            reject(error);
          }
          if (resultSet && resultSet[0]) {
            const result = resultSet[0];
            if ((result as ResultSetError).error) {
              reject((result as ResultSetError).error);
            } else {
              resolve(result as unknown as ResultSet);
            }
          }
        }
      );
    });
  }

I still have a AsyncSqlTransaction class that provides an executeSqlAsync method but does not track the transactions. I coded it so it does not need a AsyncDatabase but it meant I had to copy and paste the code for executeSqlAsync

import type { ResultSet, ResultSetError, WebSQLDatabase } from 'expo-sqlite';

export class AsyncSQLTransaction {
  constructor(private db: WebSQLDatabase, private readOnly = false) {}

  /**
   * This is the same logic as in SQLiteAsyncDatabase, but the database that is
   * passed into this transaction is NOT a SQLiteAsyncDatabase but a WebSQLDatabase
   * for interop.
   * @param sqlStatement
   * @param args
   * @returns
   */
  async executeSqlAsync(
    sqlStatement: string,
    ...args: any
  ): Promise<ResultSet> {
    return new Promise<ResultSet>((resolve, reject) => {
      this.db.exec(
        [{ sql: sqlStatement, args }],
        this.readOnly,
        (error, resultSet) => {
          if (error) {
            reject(error);
          }
          if (resultSet && resultSet[0]) {
            const result = resultSet[0];
            if ((result as ResultSetError).error) {
              reject((result as ResultSetError).error);
            } else {
              resolve(result as unknown as ResultSet);
            }
          }
        }
      );
    });
  }
}

The transactions are wrapped using txn and rtxn which are analogues to transaction and readtransaction which I coded as

  /**
   * Creates a transaction and executes a callback passing in the transaction wrapped with an async API
   * @param callback callback function that would get a transaction that provides async capability.  The return value of the callback will be the return value of this method.
   */
  async txn<T>(callback: AsyncTransactionCallback<T>): Promise<T> {
    try {
      await this.executeSqlAsync('begin transaction');
      const tx = new AsyncSQLTransaction(this);
      const rs = await callback(tx);
      await this.executeSqlAsync('commit');
      return rs;
    } catch (error) {
      await this.executeSqlAsync('rollback');
      throw error;
    }
  }

[nolanlawson]

I'm glad you found a solution. I may never get around to solving this bug, especially since it's somewhat against the spirit of the library, as I said years ago that my goal was just to emulate the WebSQL API as designed (circa 2010!) and not "carry the flame" of WebSQL.

I'll leave this issue open, though, as it seems potentially solvable without breaking backwards compatibility semantics.

[nolanlawson]

Added a failing test for this issue: https://github.com/nolanlawson/node-websql/commit/aaf5c0f6e6ee60cd8e85212efde250ebcb842042

[DerGuteMoritz]

FWIW, we were able to make this work by patching in a way to disable the auto-commit mechanism and adding explicit commit and rollback methods to WebSQLTransaction. We also extended the test suite accordingly. @nolanlawson As far as I understand you wouldn't be interested in a PR to that effect, right?

[trajano]

@DerGuteMoritz would it work in Expo?

[DerGuteMoritz]

@trajano Pretty sure, yes. IIRC the patch that Expo originally vendored node-websql for has since been upstreamed. We're using it with https://github.com/craftzdog/react-native-sqlite-2 rather than Expo's sqlite module which definitely works.

[nolanlawson]

@DerGuteMoritz

As far as I understand you wouldn't be interested in a PR to that effect, right?

Correct, I would prefer not to add new extensions to the WebSQL API. The goal of this project is to pretty narrowly just match the original WebSQL semantics and not invent new ones. Microtasks are kind of fuzzy, though, as the original WebSQL API predated promises.

[trajano]

@DerGuteMoritz from the way your code looks, it appears that it requires bare workflow is there a specific place I can discuss your SQLite implementation since it's outside the scope of this project.