JasperFx / marten

.NET Transactional Document DB and Event Store on PostgreSQL
https://martendb.io
MIT License
2.83k stars 447 forks source link

Marten does not work with TransactionScope #814

Closed mariusin79 closed 6 years ago

mariusin79 commented 7 years ago

I hope I am wrong on this one:

    [Test]
    public void Does_Marten_support_transactions()
    {
        var store = DocumentStore.For(options => { options.Connection("..."); });

        // the transaction scope does not call "Complete" and should be rolled back
        using (var scope = new TransactionScope())
        {
            using (var session = store.LightweightSession())
            {
                session.DeleteWhere<FooBarDocument>(f => true);
                session.Store(new FooBarDocument { Id = 4 });
                session.SaveChanges();
            }
        }

        // this fails, the document has somehow been persisted
        using (var session = store.LightweightSession())
        {
            var doc = session.Query<FooBarDocument>().FirstOrDefault();
            Assert.That(doc, Is.Null);
        }

    }

    public class FooBarDocument
    {
        public int Id { get; set; }
    }

I also tried adding "Enlist" to the connection string, but then the SaveChanges blows up with "nested transactions not supported". Can't seem to find the docs for this either, so how exactly does one use Transactions in Marten?

jeffdoolittle commented 7 years ago

Any calls to IDocumentSession.Store or IDocumentSession.Delete are batched together and saved transactionally once a call is made to IDocumentSession.SaveChanges. However, there is not a way to include reads in a transaction using Marten.

See http://jasperfx.github.io/marten/documentation/documents/basics/persisting/ for more details.

mariusin79 commented 7 years ago

Thats a huge problem for us, because Marten does not live in a bubble. We are using messaging together with Marten and we have to guarantee at least once delivery. Is there any way to disable transaction management in Marten so we can open/close transactions ourselves?

RagingKore commented 7 years ago

@mariusin79 Dude... I think the idea of having a document session is to be able to use a unit of work to store/update/delete data atomically when you run session.SaveChanges(). Why would you ever want to wrap it in a TransactionScope? And what does it have to do with "at-least-once-delivery"? Just curious...

deepforest commented 7 years ago

hi, We also need to use TransactionScope. From the NpgsqlConnection documentation it's said that: .BeginTransaction() "Currently there's no support for nested transactions. Transactions created by this method will have Read Committed isolation level."

so it will be nice to not calling .BeginTransaction()/Commit/Rollback at all, based on configuration, so we can create our own root transaction using TransactionScope.

As for the comment to @mariusin79 "Why would you ever want to wrap it in a TransactionScope", you want to have a transaction scope whenever you need to coordinate more than one Marten't unit-of-works, or multiple transactional resources, such as the queue suggested in a single transaction. If one fails, all fails and rollback changes.

jeremydmiller commented 7 years ago

@deepforest @mariusin79 I'd happily take a pull request to add that support to Marten. It's just not something that we've ever needed. We don't use distributed transactions in our service bus usage.

deepforest commented 7 years ago

@jeremydmiller thanks for your kind offer, but note that it's not about distributed transactions, it's about enlisting a local, same thread, transaction, created from the outside.

jeremydmiller commented 7 years ago

@deepforest Yes, and that is referred to as a distributed transaction. To be clear, I'm not planning to take this on, but I will take pull requests to add it if the code is acceptable. If you're just here to complain, please feel free to use a different tool.

roji commented 7 years ago

Seems like Marten should be checking for the existence of an enlisted ambient transaction and if so, avoid create its own transaction...

petersondrew commented 7 years ago

If you're targeting Marten 2.0, it looks like providing your own ITenant impl would be a good place to start, from there you can instantiate your own IManagedConnection and try to enlist in existing transactions there if any exist. If I'm reading the code right, that will allow any sessions you create to pick up on that IManagedConnection and enlist in your existing transaction.

mariusin79 commented 7 years ago

@jeremydmiller can you elaborate on your service bus usage? Do you have requirements for at least once delivery of messages or is it acceptable with edge cases where your persist your data and not tell the world about it (lost messages)?

In my previous projects I've tackled at least once delivery requirements using one of the following:

  1. DTC

Distributed transaction spanning msmq and the business operations being performed.

  1. Outbox pattern

With transports like Azure Service Bus and RabbitMq who does not support distributed transactions, I've used something kalled the "outbox pattern" (https://docs.particular.net/nservicebus/outbox/). The minimum requirement for this pattern is the ability to persist a message to the same data store as the business operations; meaning a shared non-dtc transaction.

I can't do option 1 and I can't do option 2, since Marten insist on starting and completing the transaction on SaveChanges (hence the bubble comment above). If you have some tips as to how this problem could be tackled I would be glad to hear it.

jeffdoolittle commented 7 years ago

If you set the IsolationLevel to IsolationLevel.Serializable then Marten starts a transaction when BeginSession is called, rather than waiting for a call to SaveChangesAsync.

see: https://github.com/JasperFx/marten/blob/a9e4ee7ca96f2ef9d82584cebfc5ee24b581ead8/src/Marten/Services/ManagedConnection.cs#L121

        public void BeginSession()
        {
            if (_isolationLevel == IsolationLevel.Serializable)
            {
                BeginTransaction();
            }
        }

and note that buildConnection, while called multiple times, does nothing if there is already a connection initialized for the ManagedConnection

see: https://github.com/JasperFx/marten/blob/a9e4ee7ca96f2ef9d82584cebfc5ee24b581ead8/src/Marten/Services/ManagedConnection.cs#L33

jeremydmiller commented 7 years ago

@mariusin79 That's a longer discussion;) Our service bus is using a store and forward queue, so the message "gets" to the destination as a discrete step from being successfully processed. The error handling could send a message back to the original caller after the maximum number of attempts are exceeded. My preference -- not that we've done this much -- would be to use compensating actions rather than have to use distributed transactions.

Not sure if every one here is just politely ignoring me, but I'll happily take a pull request to enable this as an opt in feature ;-) I don't think it'd really be that big of a deal to add.

janovesk commented 7 years ago

How do you make sure the "store" part of the store and forward queue operation is completed in sync with any business data you wrote as part of handling the business operation that caused you to send something to the queue, @jeremydmiller?

jeremydmiller commented 7 years ago

@janovesk We don't. It's in a local retry loop separate from the business tx. I can't say that we have ever had any cases where that's been a problem for us. If it did ever become an issue, we'll opt to use compensating transactions instead.

jeremydmiller commented 6 years ago

Meh, this isn't even supported in netstandard anything yet, so it'd be a .Net 4.6 only feature with all the requisite conditional compilation. Someone really needs to want this one to push through on this.

And arguably, you could pull this off yourself now w/ the very latest changes coming in 2.4.0 that allow you to pass in an existing connection. Maybe I come back w/ a .Net 4.6 only set of extension methods for that next week.

roji commented 6 years ago

AFAIK netstandard20 definitely does support System.Transactions (i.e. TransactionScope), it simply doesn't support distributed transactions (since the current .NET Framework is tied to the Windows-specific MSDTC). There's a very important distinction between the two, and I think proper TransactionScope support is quite an important feature for any .NET database product.

mariusin79 commented 6 years ago

@jeremydmiller does the support for passing in an existing connection imply that you will not start a transaction in SaveChanges? I think the DTC-part of this feature is a dead-end and not something to waste time on, but shared non-dtc transaction support would solve important things like:

  1. starting a transaction scope around an integration test (rollback data)
  2. using Marten with other technology that communicates with the postgres-database in a single transactional unit of work. Can't stress this one enough :-)
jeremydmiller commented 6 years ago

@mariusin79 "Can't stress this one enough :-)" -- agreed, but Marten has been able to do that since the very earliest pre 1.0 releases, the only thing that is different now is being able to pass in an already open connection & tx instead of accessing the one exposed by a Marten session.

I'm not actually a fan of your 2. up there, but yeah, I can see that.

At this point it's just going to be some conditional extensions to either pass in a TransactionScope or declare that you want to enlist w/ the ambient one. We'd add conditional logic into ManagedConnection to know that it's already in a TX and that it doesn't control the TX (part of that is already there)

jeremydmiller commented 6 years ago

Done. Will be in 2.4.0 for Netstandard2 or .Net 4.6