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

feat: allow nested transactions #24

Closed
wants to merge 2 commits into from

Conversation

mrnagydavid
Copy link
Contributor

In this PR, I attempted to add support for nested transactions
which means that when the consumer runs nested transactions, the inner transactions are using the same transaction as the outermost transaction.

Copy link
Member

@kirillgroshkov kirillgroshkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First concern: it'll "group" the data from different users/requests into the same Transaction, then commit it multiple times. Sounds like a no-go approach to me.

If things are to be grouped, they should be from the same Request

@kirillgroshkov
Copy link
Member

And at first glance, I don't see a simple solution to this problem, without sharing/passing a "context object" (like a backend Request)

@mrnagydavid
Copy link
Contributor Author

mrnagydavid commented Nov 15, 2024

First concern: it'll "group" the data from different users/requests into the same Transaction, then commit it multiple times. Sounds like a no-go approach to me.

True.
Commiting should be elevated into the CommonDao from CommonDb.
When the function that created the 1st transaction layer finishes, then it can be committed.
Also seems like an easy fix, although rippling through all DB implementations.

And at first glance, I don't see a simple solution to this problem, without sharing/passing a "context object" (like a backend Request)

Maybe so, although I don't grasp that.
The goal is not to have to pass around anything.
You can open a transaction whenever, and if there is already an ongoing transaction, you are going to be part of it.

Not that we should promote a lot of nested transactions, but
it would remove the need to pass tx around.

    dao.runInTransaction(async tx => {
        await tx.save(dao, items[0]!)
        await tx.save(dao, items[1]!)
        await dao2.runInTransaction(async tx2 => {
          await tx2.save(dao, items[3]!)
          await tx2.deleteById(dao, items[1]!.id)
          throw new Error('I should not have done that!')
        })
        await tx.save(dao, items[4]!)
      })

Dao2 didn't need to know anything about already being inside a transaction.

Not 100% sure but I think that with a bit more effort, it would also be possible to always use the transaction inside a transaction:

dao.runInTransaction(async () => {
  dao.save() // <- uses the transaction, b/c it's statically available on the baseclass
  dao2.runInTransaction(async () => {
    dao2.save() // <- also uses the same transaction
  })
})

@mrnagydavid mrnagydavid deleted the feat/allow-nested-transactions branch November 15, 2024 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants