Papooch / nestjs-cls

A continuation-local storage (async context) module compatible with NestJS's dependency injection.
https://papooch.github.io/nestjs-cls/
MIT License
453 stars 29 forks source link

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

Closed fredsensibill closed 3 months ago

fredsensibill commented 4 months ago

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 commented 4 months ago

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.

fredsensibill commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

Papooch commented 3 months ago

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 commented 3 months ago

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/transactional@2.4.2