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

Add an option to forbid active transactions in specific methods (@nestjs-cls/transactional) #166

Closed
fredsensibill opened this issue Jul 24, 2024 · 8 comments · Fixed by #167
Labels
enhancement New feature or request solved The question was answered or the problem was solved

Comments

@fredsensibill
Copy link

I'd like to mark some parts of my application as "transaction free", because they can take some time and they shouldn't have an active transaction while running. For example, if I'm communicating with external APIs with a transaction open that can cause DB deadlocks if the external API takes too long.

If I could mark those methods with something like @ForbidTransaction I could make sure that there are no active transactions. The decorator could throw an error if the method is called with an active transaction.

@Papooch
Copy link
Owner

Papooch commented Jul 24, 2024

This is already supported, please refer to the Transaction propagation section of the docs.

You're probably interested in the Propagation.NotSupported or Propagation.Never mode.

@Papooch Papooch added the question Further information is requested label Jul 24, 2024
@fredsensibill
Copy link
Author

At first I thought Never was what I wanted, but not really.

NotSupported: Run without a transaction even if one exists.

If I understand this correctly, the transaction is still active even if we don't use it. Active transactions that take too long to finish can cause deadlocks.

Never: Throw an exception if an existing transaction exists, otherwise create a new one

This will throw an error if there's an active transaction, which is what I want, but if there are no active transactions this will create a new one. I'd like to have a guarantee that there are no active transactions at all.

@Papooch Papooch added enhancement New feature or request and removed question Further information is requested labels Jul 24, 2024
@Papooch
Copy link
Owner

Papooch commented Jul 24, 2024

Hm, you're right, there currently isn't a propagation mode that satisfies your scenario.

I'll have a look how the Spring framework handles it (the current set of propagation modes is straight up copied over from their implementation) so I'm not reinventing the wheel.

If I don't find a satisfying answer, I'd propose a new mode NotAllowed (as a more strict version of NotSupported) which doesn't use a transaction and throws error if one exists.

What do you think?


Currently, if you want to implement such functionality in the userland, you can check if a transaction is currently active by calling isTransactionActive() on TransactionHost and throw error if so.

@fredsensibill
Copy link
Author

Well I guess it depends if you want to call this a type of "transaction propagation", because it's not really about propagation. @Transactional(Propagation.NotAllowed) kind of reads like "propagation is not allowed" which, semantically, seems very similar to @Transactional(Propagation.Never).

That's why I proposed a new decorator, it just seems like a separate thing from propagation. But hey, maybe I'm just nitpicking and @Transactional(Propagation.NotAllowed) is clear enough.

@Papooch
Copy link
Owner

Papooch commented Jul 25, 2024

I don't feel particularly eager to adding a completely new decorator for this single use case and widening the library's API surface.

But you're right that it's not exactly about propagation and I was also hesitant to call it that.

On the other hand, it also is kind of related to propagation. Maybe we just need to find a more fitting name.

I mean, to be completely honest, the names of the other modes are not exactly descriptive either :D I just didn't want to diverge from an established naming

"Transaction propagation to this method is not allowed" doesn't feel that off to me after all. Or maybe "forbidden", "rejected"?

@fredsensibill
Copy link
Author

I'm casting my vote to "rejected". My favourite so far.

@Papooch
Copy link
Owner

Papooch commented Jul 26, 2024

It turns out that I probably misinterpreted the Never propagation mode when I initially implemented it. In Spring, it seems to work exactly as you want: https://docs.spring.io/spring-framework/docs/6.1.11/javadoc-api/org/springframework/transaction/annotation/Propagation.html, i.e. it does not create a transaction if none exists. (I got probably confused by the fact, that Hibernate always creates a logical transaction, but not a physical one)

The fix is simple, this line should be changed to withoutTransaction, but seeing as aligning this behavior is a breaking change, I'll probably need to publish a new major version of @nestjs/transactional.

But at the same time, I can't imagine a scenario where one needs to use the Never mode in the current implementation.

@Papooch Papooch linked a pull request Jul 26, 2024 that will close this issue
@Papooch
Copy link
Owner

Papooch commented Aug 6, 2024

In the end I decided that the original behavior of Never was wrong, so I created a fix release.

From now on, the Never propagation mode will throw an error when a transaction is active, and will not start a new transaction.

Released in @nestjs-cls/[email protected]

@Papooch Papooch added the solved The question was answered or the problem was solved label Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request solved The question was answered or the problem was solved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants