Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Whether the transaction operates normally when promise.all is used #16603

Open
cokepizza opened this issue Oct 5, 2023 · 4 comments
Open

Whether the transaction operates normally when promise.all is used #16603

cokepizza opened this issue Oct 5, 2023 · 4 comments

Comments

@cokepizza
Copy link

cokepizza commented Oct 5, 2023

Is there a chance that the transaction won't work properly if I use promise.all?

When testing with a lot of data, we found that occasionally some tables would be committed and others would be rolled back.

Is my test wrong?

The rough code is like this:

   try {
      await Promise.all([
        Table1.update(
          {},
          {
            transaction,
          }
        ),
        Table2.increment(
          {},
          {
            transaction,
          }
      ]);
      await transaction.commit();
    } catch (e) {
      await transaction.rollback();
    }

There seems to be a similar phenomenon in other ORMs, so I wonder if sequelize will also have this issue.
https://stackoverflow.com/questions/67988319/is-there-any-reason-to-use-promise-all-in-a-typeorm-transaction

@cokepizza cokepizza added pending-approval Bug reports that have not been verified yet, or feature requests that have not been accepted yet type: bug labels Oct 5, 2023
@ephys
Copy link
Member

ephys commented Oct 5, 2023

Queries run in transactions are always run sequentially because transactions are per connection and only one query can be run on a single connection at a time

This means that if update runs first but errors, the transaction could be rolled back while increment is still queued to be run, making it theoretically possible for that query to run outside of the transaction. I have not encountered this issue but I think it's possible for it to happen.

They are queued by the connector library (pg, tedious, etc), so they are not aware of the state of our transaction object. I think we could prevent this problem from happening by having our own queue in the transaction, in which queries that belong to the transaction are put and queued, and discard them if the transaction closes

@ephys ephys removed the pending-approval Bug reports that have not been verified yet, or feature requests that have not been accepted yet label Oct 5, 2023
@cokepizza
Copy link
Author

Thank you for your quick reply. If it works as you said, there is a possibility that the transaction is not applied properly. There was advice in another article to use Promise.allsettled, so I'll try it and see if it solves the problem. thank you!

@cokepizza
Copy link
Author

As a result of the test, when promise.allsettled was used, not a single transaction-related issue occurred out of approximately 50,000 requests.

I replaced the existing promise.all by creating a function as follows:

export const promiseTx = async (promiseArr: any[]) => {
  const settled = await Promise.allSettled(promiseArr);

  for (const res of settled) {
    if (res.status === 'rejected') {
      throw res.reason;
    }
  }

  return settled.map((item) => item.status === 'fulfilled' && item.value);
};

@hlhdaiaaii
Copy link

hlhdaiaaii commented Dec 22, 2023

Thanks a lot. This confirmation is exactly what I need. It took me hours figuring why rollback does not revert the whole changes at all when using Promise.all and whether it is due to either sequelize or database problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants