Add support for System.Transactions #9561

AndriySvyryd · 2017-08-24T23:10:18Z

The tests currently only run for SQL Server on net461

ajcvickers · 2017-08-25T19:38:07Z

src/EFCore.Relational.Specification.Tests/TransactionTestBase.cs

        {
            using (var transaction = TestStore.BeginTransaction())
            {
                using (var context = CreateContextWithConnectionString())
                {
-                    context.Database.AutoTransactionsEnabled = autoTransaction;


Why did this change?

Redundant test. AutoTransactionsEnabled only affects SaveChanges. This test doesn't call SaveChanges.

ajcvickers · 2017-08-25T19:42:47Z

src/EFCore.Relational/Storage/RelationalConnection.cs

@@ -104,6 +104,53 @@ protected RelationalConnection([NotNull] RelationalConnectionDependencies depend
        public virtual IDbContextTransaction CurrentTransaction { get; [param: CanBeNull] protected set; }

        /// <summary>
+        ///     The currently enlisted transaction.


I think we need more here to document what flavor of .NET transaction this "Transaction" type is. @divega?

System.Transactions.Transaction?

@divega I was thinking of some more words than this to help people decide which to use...

Ok. I misunderstood. Stepping back I think these API additions can be very confusing. Not sure additional words in the member documentation are going to help.

I am thinking we should brainstorm something we can use in the API names as well to differentiate from the normal scenarios we already support. Right now "external transaction" comes to mind.

Another approach suggested by @anpete would be to make these extension methods in the System.Transactions namespace, so people have to add a using statement to light the API up.

ajcvickers · 2017-08-25T19:47:31Z

src/EFCore.Relational/Storage/RelationalConnection.cs

+                    // the connection. This will enlist the newly opened connection to the second transaction but will also close the reader being used
+                    // by the first active query. As a result when trying to continue reading results from the first query the user will get an exception
+                    // saying that calling "Read" on a closed data reader is not a valid operation.
+                    AmbientTransaction = current;


I may be missing something here, but is this better than just throwing? In other words, what's the valid scenario where we need to do this?

This is EF6 behavior. If the query reader enlisted in the first transaction is never read from after this the second operation will still work. We can choose to throw instead.

Not sure, would like to discuss with @divega

ajcvickers

Approving with the changes to close/open discussed in person.

AndriySvyryd · 2017-09-01T00:03:55Z

@divega, @ajcvickers I've updated with extension methods and connection keep-alive. Can you review again?

I found a scenario broken by the keep-alive:

using (new TransactionScope())
{
    // query

    using (new TransactionScope(TransactionScopeOption.Suppress))
    {
        // query
    }

    // SaveChanges
}

Now the middle query would still be executed in the transaction. If we tried to unenlist it would throw.

divega · 2017-09-01T05:27:07Z

I found a scenario broken by the keep-alive:...

Is this a scenario in which you would really need more than one connection? Then it is kind of nice if it throws...

ajcvickers · 2017-09-07T16:24:27Z

Signing off on the latest changes.

divega

CC @roji, @ErikEJ

divega · 2017-09-07T19:39:50Z

Upgrading to providers-beware since there is new API here (SupportsAmbientTransactions) providers would need to implement.

Fixes #5595

bruno-garcia · 2017-10-09T21:12:41Z

No milestone set so I wonder: When could we expect this on NuGet?

AndriySvyryd · 2017-10-09T21:14:21Z

@bruno-garcia This is just a pull request, the corresponding issue is #5595 and it's in the 2.1.0 milestone

bruno-garcia · 2017-10-10T08:37:10Z

My bad. I got to the issue after asking here.

codematrix · 2018-03-31T20:08:37Z

So, I guess this is still not supported? I'm currently using 2.02 and still getting the same issue.

AndriySvyryd · 2018-03-31T20:16:45Z

@codematrix This feature is in 2.1.0-preview1

roji · 2018-05-13T10:05:52Z

FYI, am adding support for this in Npgsql and wanted to share some info.

Some of the tests purposefully generated an error and then continue with the TransactionScope (SaveChanges_uses_enlisted_transaction, SaveChanges_uses_ambient_transaction_with_connectionString, SaveChanges_uses_ambient_transaction when closeConnection is true). PostgreSQL However, in PostgreSQL, a transaction is put into a failed state after any error, and is unrecoverable - it can only be rolled back. So these tests must be skipped for now (which isn't great because they test some important behavior).

Ideally these tests wouldn't depend on continuing after a failure. But it's no big deal - mostly wanted you to know that you cannot rely on a transaction being recoverable after an error across databases.

ajcvickers · 2018-05-14T19:21:57Z

@AndriySvyryd Should we file a test issue for this?

AndriySvyryd · 2018-05-14T19:40:51Z

Filed #12001

AndriySvyryd added the providers-fyi label Aug 24, 2017

AndriySvyryd requested review from bricelam, divega and ajcvickers August 24, 2017 23:10

dnfclas added the cla-already-signed label Aug 24, 2017

ajcvickers reviewed Aug 25, 2017

View reviewed changes

AndriySvyryd force-pushed the Issue5595 branch 3 times, most recently from 7e0f505 to 0d00876 Compare August 25, 2017 21:19

ajcvickers approved these changes Aug 25, 2017

View reviewed changes

AndriySvyryd force-pushed the Issue5595 branch 8 times, most recently from b6d4663 to 810aa01 Compare August 31, 2017 23:57

AndriySvyryd force-pushed the Issue5595 branch 2 times, most recently from d897a89 to 6bf389d Compare September 5, 2017 17:55

divega approved these changes Sep 7, 2017

View reviewed changes

divega added the providers-beware label Sep 7, 2017

Add support for System.Transactions

792ee4d

Fixes #5595

AndriySvyryd force-pushed the Issue5595 branch from 6bf389d to 792ee4d Compare September 7, 2017 22:56

AndriySvyryd merged commit 792ee4d into dev Sep 7, 2017

AndriySvyryd deleted the Issue5595 branch September 7, 2017 22:56

roji mentioned this pull request Sep 10, 2017

System.Transactions support npgsql/efcore.pg#242

Closed

sebitsi mentioned this pull request Mar 19, 2018

Using DbContext in nested scope aspnetboilerplate/aspnetboilerplate#3210

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for System.Transactions #9561

Add support for System.Transactions #9561

AndriySvyryd commented Aug 24, 2017

ajcvickers Aug 25, 2017

AndriySvyryd Aug 25, 2017

ajcvickers Aug 25, 2017

divega Aug 25, 2017

ajcvickers Aug 25, 2017

divega Aug 28, 2017

divega Aug 28, 2017

ajcvickers Aug 25, 2017

AndriySvyryd Aug 25, 2017 •

edited

Loading

ajcvickers Aug 25, 2017

ajcvickers left a comment

AndriySvyryd commented Sep 1, 2017

divega commented Sep 1, 2017

ajcvickers commented Sep 7, 2017

divega left a comment

divega commented Sep 7, 2017

bruno-garcia commented Oct 9, 2017

AndriySvyryd commented Oct 9, 2017

bruno-garcia commented Oct 10, 2017

codematrix commented Mar 31, 2018

AndriySvyryd commented Mar 31, 2018

roji commented May 13, 2018

ajcvickers commented May 14, 2018

AndriySvyryd commented May 14, 2018

Add support for System.Transactions #9561

Add support for System.Transactions #9561

Conversation

AndriySvyryd commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndriySvyryd Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajcvickers left a comment

Choose a reason for hiding this comment

AndriySvyryd commented Sep 1, 2017

divega commented Sep 1, 2017

ajcvickers commented Sep 7, 2017

divega left a comment

Choose a reason for hiding this comment

divega commented Sep 7, 2017

bruno-garcia commented Oct 9, 2017

AndriySvyryd commented Oct 9, 2017

bruno-garcia commented Oct 10, 2017

codematrix commented Mar 31, 2018

AndriySvyryd commented Mar 31, 2018

roji commented May 13, 2018

ajcvickers commented May 14, 2018

AndriySvyryd commented May 14, 2018

AndriySvyryd Aug 25, 2017 •

edited

Loading