rebus-org / Rebus.UnitOfWork

:bus: Unit of work helper for Rebus
https://mookid.dk/category/rebus
Other
2 stars 1 forks source link

Missing exception handling in UoW factory #1

Closed JornWildt closed 4 years ago

JornWildt commented 4 years ago

The code in UnitOfWorkStep.Process() goes like this:

        var unitOfWork = await _unitOfWorkFactoryMethod(messageContext);

        try
        {
            await next();
            await _commitAction(messageContext, unitOfWork);
        }
        catch (Exception)
        {
            await _rollbackAction(messageContext, unitOfWork);
            throw;
        }
        finally
        {
            await _cleanupAction(messageContext, unitOfWork);
        }

That means we have some nice exception handling with cleanup around commit and rollback. But why not handle exceptions from the factory method?

The issue is that the factory method, in our case (but is seems rather natural), is responsible for setting up the transation ... and sometimes that fails, possibly because of network errors or other infrastructure issues.

When the factory method fails, cleanup is not called - leading to various wonderful and catastrophic error conditions afterwards.

Out fix is to add our own cleanup handling in the factory method - but it should be handled by the bus infrastructure code calling _cleanupAction().

mookid8000 commented 4 years ago

If the construction of the unitOfWork instance fails, what do you propose should be passed to the _cleanupAction function?

JornWildt commented 4 years ago

If the construction of the unitOfWork instance fails, what do you propose should be passed to the _cleanupAction function?

Ah, good point, there won't be any UoW to pass to it, right.

I don't have the code right here (on vacation), but I don't think the UoW is used for anything as all our infrastructure depends on the ambient transaction maintained by Rebus.

I think the cleanup code simply closes the database connection - which would work even with UoW being passed as null. That was actually our main issue, having database connections that was not closed when start-transaction failed.

mookid8000 commented 4 years ago

I'll close this issue now, as there's not really anything the uow plugin can do if the call to _unitOfWorkFactoryMethod fails, because is doesn't have anything to clean up.

It follows from this that it's important that a failure during the creation of the uow instance does not leak resources.