microservices-patterns / ftgo-application

Example code for the book Microservice patterns
Other
3.35k stars 1.29k forks source link

Saga failures and order creation breaking - one of the reasons and solution #113

Closed asn25 closed 3 years ago

asn25 commented 3 years ago

Hello,

I have two points on the topic:

1) I did some research regarding issue #70 and situation when Sagas stop working at all and orders are always created at APPROVED_PENDING state. It's connected with documented already Exception: "org.springframework.orm.jpa.JpaSystemException: Transaction was marked for rollback only; cannot commit; nested exception is org.hibernate.TransactionException: Transaction was marked for rollback only; cannot commit"

On my thoughts, if thrown, this exception seems to break JPA provider work and sagas stop working (saga can't persist itself on JDBC level - ?).

I found the point in code where this happens. It is domain service with annotation @Transactional (i.e. all methods of domain service are transactional), in particular, as for issue #70 (revise cancelled order issues) it's about OrderService.beginReviseOrder(). The problem is that Aggregate call inside (order.revise) throws UnsupportedStateTransitionException. And it gives JpaSystemException: Transaction was marked for rollback only.

My solution is to set Propagation.REQUIRES_NEW on the OrderService.beginReviseOrder() method:

@Transactional(propagation = Propagation.REQUIRES_NEW) public Optional beginReviseOrder(long orderId, OrderRevision revision) { return orderRepository.findById(orderId).map(order -> { ResultWithDomainEvents<LineItemQuantityChange, OrderDomainEvent> result = order.revise(revision); orderAggregateEventPublisher.publish(order, result.events); return new RevisedOrder(order, result.result); }); }

This fix fixes the issue #70 (and might be a number of other issues connected with Sagas). It just works as it should. Issue #70 is not breaking the system any more (revise cancelled order), Saga just rolls back and all is working further (new orders creation, sagas). No need to restart all the system. Hopefully, this might fix issues #38, #111 as well.

So it's needed to understand whether to make all domain services methods as Propagation.REQUIRES_NEW (because almost every domain service method contains Aggregate method call that might throw the exception). Or maybe there is another solution.

Here is some information on "Transaction marked as rollbackOnly" issue: https://stackoverflow.com/questions/19302196/transaction-marked-as-rollback-only-how-do-i-find-the-cause https://vcfvct.wordpress.com/2016/12/15/spring-nested-transactional-rollback-only/

====

2) Also I made all CommandHandlers of all saga participants handler methods wrapped with try-catch, and catch top level Exception. To minimize the chance that Exception will rise up and break Saga. (But it will not help with "JpaSystemException: Transaction was marked for rollback only". With this exception it's needed to sort out with nested @Transactional calls or Propagation). Example (OrderCommandHandlers.beginReviseOrder()):

public Message beginReviseOrder(CommandMessage cm) { try { long orderId = cm.getCommand().getOrderId(); OrderRevision revision = cm.getCommand().getRevision(); return orderService.beginReviseOrder(orderId, revision) .map(result -> withSuccess( new BeginReviseOrderReply(result.getChange().getNewOrderTotal()))) .orElseGet(CommandHandlerReplyBuilder::withFailure); } catch (Exception e) { System.out.println("\n --- OrderCommandHandlers.beginReviseOrder fail block --- " + e.getClass().getCanonicalName() + ": " + e.getClass().getCanonicalName() + ": " + e.getMessage() + "\n"); e.printStackTrace(); return withFailure(); }

Thanks

cer commented 3 years ago

Message handers should not use @Transactional - that interferes the transaction managed by Tram.

asn25 commented 3 years ago

And message handlers do not use @transactional. The excerpt with @transactional in my post was domain service - OrderService.beginReviseOrder().

All domain services in FTGO are @Transactional (except AccountingService).

asn25 commented 3 years ago

If I remove @transactional at all from domain service, it also works - issue #70 is not appearing and "JpaSystemException: Transaction was marked for rollback only" is not thrown (on attempt to revise cancelled order). So sagas work OK.

But the question is - whether it's acceptable to remove @transactional from domain services? Because domain services do publishing of domain events, and this should be done in one transaction with "repository.save()" operation (according to "Transactional messaging" pattern). And Saga should also start in one transaction with "repository.save()" - in theory...

You mentioned that Eventuate Tram do transaction itself under the hood, so it the case of message handling - possibly it should be OK if domain service is not transactional. But domain service could be used not only from message handler, so - in theory - it should be transactional.

asn25 commented 3 years ago

The same problem as #70 happens if try to cancel cancelled order: "org.springframework.orm.jpa.JpaSystemException: Transaction was marked for rollback only" is thrown and system is not in the valid state anymore. Analogously, if we set @Transactional(propagation = Propagation.REQUIRES_NEW) on OrderService.beginCancel(), then it helps and works.

So common issue description is: MessageHandler -> domain service (@transactional) -> aggregate(throws exception)

Exception in Aggregate leads to "Transaction was marked for rollback only" to the CURRENT TRANSACTION (even if Aggregate exception is caught somewhere), and it seems by default this current transaction is a TRAM transaction. And then Saga can't commit after final step (TRAM transaction fails to commit).

asn25 commented 3 years ago

Interesting fact:

Looks like Saga orchestrator loads and re-executes tasks that failed on previous version of service.

asn25 commented 3 years ago

As for me, for test purposes, the simplest fix of this issue is to put REQUIRES_NEW on the head of all domain service classes:

@Transactional(propagation = Propagation.REQUIRES_NEW) public class OrderService {

Probably, could be optimized (f.e. for findById()-like-methods could be set as REQUIRED or none).

cer commented 3 years ago

OrderService. beginReviseOrder() is invoked by the messaging handling logic. These methods cannot be @Transactional (nor can they belong to an @Transactional class).

asn25 commented 3 years ago

Will it be fixed it commit?

The issue is closed but the bug is present in the application.