Transaction does not properly rollback when using `Promise.all`

[mvelzel]

This is related to this issue: #899, but that issue was marked invalid and closed.

When using an await Promise.all construction inside a transaction, an error that is thrown inside the one of the promises causes the await Promise.all to fail, but the transaction does not properly get rolled back.

I managed to create a reproducible example in the form of a unit test here:

describe("Transactions", () => {
  beforeEach(async () => {
    await db.schema
      .createTable("transactions_promises_tests")
      .addColumn("name", "text")
      .execute();
  });

  afterEach(async () => {
    await db.schema.dropTable("transactions_promises_tests").execute();
  });

  it("rollback when a Promise inside Promise.all fails", async () => {
    const names = ["test1", "test2", "error"];

    const transaction = db.transaction().execute(async (trx) => {
      await Promise.all(
        names.map(async (name) => {
          if (name === "error") throw new Error("Test error");
          await trx
            .insertInto("transactions_promises_tests")
            .values([{ name }])
            .execute();
        }),
      );
    });

    await expect(transaction).rejects.toThrow("Test error");

    const tableContents = await db
      .selectFrom("transactions_promises_tests")
      .selectAll()
      .execute();

    // This fails
    expect(tableContents.length).toBe(0);
  });

  it("rollback when a Promise inside a for-loop containing 'await [Promise]' fails", async () => {
    const names = ["test1", "test2", "error"];

    const transaction = db.transaction().execute(async (trx) => {
      for (const name of names) {
        if (name === "error") throw new Error("Test error");
        await trx
          .insertInto("transactions_promises_tests")
          .values([{ name }])
          .execute();
      }
    });

    await expect(transaction).rejects.toThrow("Test error");

    const tableContents = await db
      .selectFrom("transactions_promises_tests")
      .selectAll()
      .execute();

    // This succeeds
    expect(tableContents.length).toBe(0);
  });
});

I first noticed this problem when using a Promise.all construction inside a migration. Then I discovered the problem applies to transactions in general. My current workaround is to rewrite all my Promise.all logic to the second construction shown in the above example, which is just a simple for (...) { await (...) } construction.

[koskimas]

The async operations you start keep running after Promise.all gets rejected. Promise.all immediately rejects when the first item is rejected. So the operations keep running after the transaction ends. The newly freed connection keeps inserting stuff.

You need to use Promise.allSettled.

[koskimas]

We should probably invalidate the transaction object and throw errors from all those outside-transaction operations though 🤔

[mvelzel]

At the very least maybe somewhere in the documentation a warning might be appropriate. The only thing I found about this online after experiencing a failed rollback for a migration was the previously mentioned ticket that was closed and marked as invalid.

Only by further investigating this myself did I find that this behaviour was fundamental to Promise.all and not my own fault. Good suggestion to use Promise.allSettled instead, did not immediately think of that.

Invalidating the transaction object and throwing errors does seems like it would be a great solution in my opinion though.

[koskimas]

Documenting how Promise.all works is not our job. We won't educate people on the basics of async programming.

But we should show errors when a committed/rolled back transaction is used. And definitely not allow the released connection, that's bound to the transaction, to be used. I actually thought we did that already. Maybe the issue only exists on certain dialects.

[SimonSimCity]

I'm a coworker of @mvelzel - we're using PostgresDialect as provided by the kysely package.

[jounii]

As side note. This is mostly the reason I personally prefer for await .. of .. syntax. It does make the calls synchronous and you can catch the reject with normal exception handling and quit the loop with different conditions other than the rejection. Firing many promises sometimes might even require some throttling or promise pooling management depending on the target concurrency capabilities.

In the example above the for (const name of names) { is actually incorrect as it will fireup all promises parallel and the test promise is resolved without the promises waited for resolving. Thus you may end up the jest/test ending before the promises are handled (unhandled promise error). Most likely it is meant to be for await (const name of names) { so the upper async is waiting for the loop to end.