Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add async support to System.Transactions (e.g. TransactionScope) #1420

Open
Timovzl opened this issue Oct 4, 2019 · 39 comments
Open

Add async support to System.Transactions (e.g. TransactionScope) #1420

Timovzl opened this issue Oct 4, 2019 · 39 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Transactions enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@Timovzl
Copy link

Timovzl commented Oct 4, 2019

TransactionScope is a disposable type that almost always has I/O work to do on disposal.

Are there any plans for making TransactionScope implement IAsyncDisposable? (I am not aware of how readily the ADO.NET methods involved are available as async.)

@StephenBonikowsky
Copy link
Member

@Timovzl We didn't have plans to do this, but we will consider it for 5.0.0. We need to do some investigation first to see how complicated the work will be.

@fredericDelaporte
Copy link

Based on the votes on this Stackoverflow question (of mine) about that, it looks to me this feature is awaited by many people.

@karlra
Copy link

karlra commented Dec 12, 2019

I don't understand either how this isn't included already. It is such a basic feature and together with https://github.com/dotnet/corefx/issues/42341, TransactionScope is basically not usable right now.

@StephenBonikowsky StephenBonikowsky transferred this issue from dotnet/corefx Jan 7, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Transactions untriaged New issue has not been triaged by the area owner labels Jan 7, 2020
@StephenBonikowsky StephenBonikowsky added this to the 5.0 milestone Jan 7, 2020
@StephenBonikowsky StephenBonikowsky removed the untriaged New issue has not been triaged by the area owner label Jan 7, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@StephenBonikowsky StephenBonikowsky removed the untriaged New issue has not been triaged by the area owner label Feb 24, 2020
@HongGit HongGit added the enhancement Product code improvement that does NOT require public API changes/additions label Mar 12, 2020
@HongGit HongGit modified the milestones: 5.0.0, Future Jul 9, 2020
@scalablecory scalablecory added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 24, 2020
@Timovzl
Copy link
Author

Timovzl commented Jul 26, 2021

@StephenBonikowsky, could this be moved up for consideration again? It is so basic, so IO-heavy, and so unusual in its lack of an async API.

@StephenBonikowsky
Copy link
Member

@HongGit and @StephenMolloy who own this feature area.

@meiedomi
Copy link

meiedomi commented Sep 10, 2021

In our project we wrap the TransactionScope inside a custom unit of work. Since we do more stuff than just dispose the TransactionScope in our own Dispose method, we switched to IAsyncDisposable. Since we worked with AsyncLocal<T> inside of our unit of work, we encountered that it was impossible to restore ambient context inside of the new DisposeAsync method, because inside that method we have already lost the original value of AsyncLocal<T> and got a copy of it instead.

As far as I can tell, there is no way to get around this.

Probably the main difficulty for supporting DisposeAsync in TransactionScope has to do with that very same issue.

@Timovzl
Copy link
Author

Timovzl commented Sep 12, 2021

@meiedomi Thanks for your addition. Could you elaborate on the issue you're having?

In my experience, the following holds true: An AsyncLocal<T> value goes out-of-scope exactly when any async method completes on a call stack depth no deeper than where the value was assigned.

In other words:

  • If we assign such a value from an async method, the scope ends when we complete that method.
  • However, if we assign such a value from a synchronous method (such as a constructor!), the value remains in scope, until we complete any async method higher up our call chain.

(As a side note, remember that we can have Task-returning methods without the async keyword, which behave like any non-async method when it comes to AsyncLocal<T> scoping.)

Are you doing anything special with regards to where your unit of work is instantiated vs. disposed?

I believe the following usage pattern should pose no problems:

public async Task PerformExampleUnitOfWork()
{
    // The TransactionScope constructor assigns the Transaction.Current AsyncLocal<T> value
    // It remains visible as we exit the constructor, since it is a non-async method
    await using (var transactionScope = new TransactionScope(/*snip*/))
    {
        // This async method is deeper down the call stack
        // As such, it does not cause the AsyncLocal<T> value to go out of scope
        await this.InteractWithDatabase();

        transactionScope.Complete();
    } // TransactionScope.DisposeAsync() still sees the same AsyncLocal<T> value :)
} // AsyncLocal<T> value goes out-of-scope, as we complete an async method on the same level or higher compared to its assignment

@meiedomi
Copy link

meiedomi commented Sep 12, 2021

@Timovzl The scope of AsyncLocal<T> is directly associated to that of the current ExecutionContext. In fact, all async local values live in a dictionary ExecutionContext.m_localValues. This execution context has copy-on-write semantics, that is, the entire thing is shallow-copied each time any AsyncLocal<T> value is modified.

The current ExecutionContext is a thread-static value which is flown across various execution points such as when threads or tasks are spawned or when async methods are invoked. Each time some thread X encounters an await statement, the thread Y which is running the continuation later on will receive the same ExecutionContext instance as X had previously. And thus, any AsyncLocal<T> will get restored with it.

Here is the pseudo code to guide the following discussion.

public class UnitOfWork : IAsyncDisposable
{
  private readonly TransactionScope scope;

  public UnitOfWork()
  {
    this.scope = new TransactionScope(/*snip*/);
    // Setup other things.
  }

  public async Task CompleteAsync()
  {
    // Flush changes to the database or whatever. Not important here.
  }

  public async ValueTask DisposeAsync()
  {
    await MyOwnCleanupAsync(); // Note: This will still make use of the current transaction scope.
    this.scope.Dispose();
  }
}

Usage looks like that:

await using (var uow = new UnitOfWork())
{
  // Do stuff like calling repositories (with or without await).
  await uow.CompleteAsync();
} // <-- DisposeAsync called here

// Disaster here: We are still in the ambient TransactionScope (which is disposed already, but still)
// created by the now disposed unit of work.

So you can see where this is going: If I start a TransactionScope (which essentially stores itself into some AsyncLocal internally) in the constructor of our unit of work uow, then that scope is stored in the current ExecutionContext. This execution context gets retained across every await inside of my await using block. Note that the body of this block is not yet async. The await in front of the using refers to the implicit call to DisposeAsync() at the end of the block. This is also where the problem lies. The compiler emits await uow.DisposeAsync(), so at this point the execution context flows unaltered into DisposeAsync. However, in there I call Dispose() of my TransactionScope (remember, my uow simply wraps a transaction scope plus adds some additional functionality that need to be disposed asynchronously). The transaction scope sets its AsyncLocal back to null (or any outer ambient transaction scope if any was present), which leads to the afore-mentioned copy-on-write of the entire execution context. This modified execution context is only visible to the current async method (i.e. the DisposeAsync) and anything called by that. However, when DisposeAsync returns, the previous execution context is restored and with it the old TransactionScope. Bang!! :(

The current implementation of TransactionScope assumes that the creation and disposal of it live in the same execution context, which is no longer the case if we switch to DisposeAsync.

The only way out for us is to not use await using and instead pass the body of the unit of work as a lambda into some higher-order function. Like that:

    public static class UnitOfWork
    {
        public static async Task RunAsync(Func<Task> uowBody)
        {
            using (var tc = new TransactionScope(/*snip*/))
            {
                await uowBody();
                await MyOwnCleanupAsync();
                tc.Complete();
            } // <-- TransactionScope is disposed in the same execution context that created it.
        }
    }

Usage:

    UnitOfWork.RunAsync(async (uow) => {
      // Do stuff with the unit of work.
      // I do not even need to complete anything, as this can be done generically in the higher order function.
    })

@Timovzl
Copy link
Author

Timovzl commented Sep 13, 2021

@meiedomi Thanks for the thorough explanation!

I might have a solution:

(As a side note, remember that we can have Task-returning methods without the async keyword, which behave like any non-async method when it comes to AsyncLocal scoping.)

The crux is that the calling method, i.e. the one holding UnitOfWork in a using, should call TransactionScope.Dispose() via a non-async intermediate method. That way, it observes TransactionScope's modification to its AsyncLocal<T>.

Easy enough to do:

public class UnitOfWork : IAsyncDisposable
{
    // Snip all the way down to DisposeAsync()...

    // WITHOUT async keyword!
    public ValueTask DisposeAsync()
    {
        // This method must remain non-async
        // This allows the caller to observe the TransactionScope's AsyncLocal modifications

        this.scope.Dispose();
        return this.DisposeAsyncCore();
    }

    private async ValueTask DisposeAsyncCore()
    {
        // Await whatever we like here

        // Snip...
    }

Because DisposeAsync is now a synchronous method (that just so happens to return an awaitable), its modifications to any AsyncLocal<T> are now observed from the nearest encapsulating async method, as though they had been made from that method itself. We have achieved the same thing as your try/finally example: for all intents and purposes, it was RunAsync that called TransactionScope.Dispose().

Please report back and share if that solves your issue!

@meiedomi
Copy link

meiedomi commented Sep 13, 2021

@Timovzl Thanks a lot for your great solution. I also learned something about async/await in the process. :)
This would have solved the problem I posed perfectly.

Unfortunately, I misrepresented our own problem. :( The stuff we do inside MyOwnCleanupAsync actually still makes use of the ambient transaction scope, i.e. it writes some things to the database which should happen in the same transaction still. Therefore, it needs to happend before the call to scope.Dispose().
I corrected my last post accordingly.

Still more ideas? :)

@Taudris
Copy link

Taudris commented Sep 13, 2021

This may work:

public class UnitOfWork : IAsyncDisposable
{
    // WITHOUT async keyword!
    public ValueTask DisposeAsync()
    {
        // This method must remain non-async
        // This allows the caller to observe the TransactionScope's AsyncLocal modifications

        // Prevent DisposeAsyncCore() from creating a copy of the ExecutionContext, so its AsyncLocal modifications are observable
        // Unknown detail: AsyncFlowControl implements IDisposable to revert flow. Does the suppression survive multiple ExecutionContext creations?
        // If not (which I suspect), then asyncFlowControl can be disposed inside this method instead of passing it to DisposeAsyncCore.
        var asyncFlowControl = ExecutionContext.SuppressFlow();

        return this.DisposeAsyncCore(asyncFlowControl);
    }

    private async ValueTask DisposeAsyncCore(AsyncFlowControl asyncFlowControl)
    {
        asyncFlowControl.Dispose(); // This can probably be moved to DisposeAsync as a using statement

        // Await whatever we like here

        this.scope.Dispose();

        // Snip...
    }
}

I don't know which method must contain the call to asyncFlowControl.Dispose(). It depends on how flow suppression is implemented which I don't have time to review just this second.

@meiedomi
Copy link

meiedomi commented Sep 13, 2021

@Taudris Wow that's pretty black magic right there! But it actually works! Thank you! :)

I wonder how I could make use of using (ExecutionContext.SuppressFlow()) inside of DisposeAsync? As far as I understand, it is crucial to restore proper flow once we entered DisposeAsyncCore, as we certainly don't want to keep suppressing the flow inside of that. Right?
Also, which other method did you think could/should host asyncFlowControl.Dispose() and why?

I have a minor but important addition to your solution. If I don't use ConfigureAwait(false) (extension method on IAsyncDisposable) on my unit of work, the continuation that runs after my unit of work got disposed might be on some random thread from the ThreadPool. Since we suppressed the flow of the execution context, this thread's context is not restored and it might therefore have a completely unrelated execution context, causing even worse kinds of side effects. The ConfigureAwait(false) makes sure that the very same thread that ends our DisposeAsyncCore actually continues after the unit of work is done.

The reason why my tests were all successful even without the extra ConfigureAwait(false) is because there is a sneaky litte optimization in place that reuses the same thread in some situations regardless. See here: https://stackoverflow.com/a/59691044/3616714. Since this is not documented or standardized, we should not rely on it and always use ConfigureAwait(false).

This solution also slightly complicates the usage of my unit of work, as I now have to return a ConfiguredAsyncDisposable from a factory instead of the unit of work directly. When I want to call other methods of the unit of work while it is executing, I have to provide it using an out parameter:

await using (UnitOfWorkFactory.Create(out var uow))
{
  await uow.CompleteAsync();
}

I the end, this starts to look equally convoluted as the solution with the higher-order function. And the "black magic" going on under the hood doesn't really give it the upper hand either. I believe that all those issues are reason enough why there is no DisposeAsync for TransactionScope yet (and probably never will be?).

@Taudris
Copy link

Taudris commented Sep 14, 2021

It seems that I misunderstand ExecutionContext. Here's me trying to test it. It seems there is stuff that async is doing with ExecutionContext that cannot be replicated without async or reflection. I'm not done experimenting, but I need to stop for now (actual work).

@Taudris
Copy link

Taudris commented Sep 15, 2021

Well, it turns out that ExecutionContext.SuppressFlow() can't be used to accomplish what I thought it was doing from reading the source code. (It is really dense stuff!) It doesn't cause future invocations of .Capture() to use the current EC (without cloning) or its AsyncLocals like I thought; it just causes .Capture() to return null until the suppression is removed, and the intended pattern for callees in that case is to not attempt to restore an EC at all and just use the current one. Which doesn't work for your use case, of course.

Here's another approach I came up with using ExecutionContext.Run(). This is still not a perfect solution, though, because it prevents any new AsyncLocal values from being visible to the methods on UnitOfWork.

At this point, I don't think a perfect solution with your desired constraints is possible. The correct solution is for the creator of UnitOfWork to manage the TransactionScope, not UnitOfWork itself. If you need UnitOfWork to be in control of the transaction, TransactionScope is not the correct tool, and you should use some other facility like DbTransaction or ORM-native transaction APIs instead.

@Timovzl
Copy link
Author

Timovzl commented Sep 15, 2021

@meiedomi As per the docs, it is ConfigureAwait( true ) that causes the original SynchronizationContext and TaskScheduler to be used for the continuation. This happens to be the default, so it may be omitted. By contrast, ConfigureAwait(false) exists specifically to indicate that this is unnecessary, as a potential performance optimization.

Regardless, I do not believe that ConfigureAwait should make a difference here. Quoting Stephen Toub in the ConfigureAwait FAQ:

I used ConfigureAwait(false), but my AsyncLocal still flowed to code after the await. Is that a bug?
No, that is expected. AsyncLocal data flows as part of ExecutionContext, which is separate from SynchronizationContext. Unless you’ve explicitly disabled ExecutionContext flow with ExecutionContext.SuppressFlow(), ExecutionContext (and thus AsyncLocal data) will always flow across awaits, regardless of whether ConfigureAwait is used to avoid capturing the original SynchronizationContext. For more information, see this blog post.

Can you confirm that the boolean passed to ConfigureAwait makes no difference? Perhaps you could test from a controller method in an ASP.NET Core application: that way, the original thread actually has a SynchronizationContext and is also not a thread pool thread, which should cause a distinct difference between ConfigureAwait(false) and ConfigureAwait(true). That should truly hit both scenarios, allowing you to observe that both have the same result, hopefully.

@Timovzl
Copy link
Author

Timovzl commented Sep 15, 2021

At this point, I don't think a perfect solution with your desired constraints is possible.

@Taudris I'm surprised. @meiedomi reported success with your original proposal of this solution. Could you describe what is wrong with that approach, and why it did seem to work?

@meiedomi
Copy link

meiedomi commented Sep 16, 2021

I can confirm that in my test scenario @Taudris original proposal still works. It also makes sense as to why it works. When entering an async method, the execution context usually gets captured. Unless it is suppressed, which is exactly what we do. Since nothing gets captured, nothing can be restored by the continuation to await DisposeAsync(). Since nothing gets restored, the execution context is whatever execution context the thread running the continuation happens to have at this point.

Since we want that thread to have the same context as the one we got right after the last await inside of DisposeAsyncCore, we must make sure that the thread is the same. And to address @Timovzl question here: This can only be guaranteed if we use ConfigureAwait(false) on the awaiter to DisposeAsync. In my test scenario, it works even with the default ConfigureAwait(true), but that's because of an optimization by .NET which still uses the same thread in most common scenarios (but this is not guaranteed in every case which is why we need to use ConfigureAwait(false) explicitly).

Here is my test scenario:

[TestFixture]
public class ExecutionContextFlowTests
{
	private readonly AsyncLocal<int> asyncLocal = new();
	
	[Test]
	public async Task TestExecutionFlow()
	{
		asyncLocal.Value = 42;

                // ConfigureAwait(false) is the only way to guarantee that thread X will run the continuation.
		await DisposeAsync().ConfigureAwait(false);
		
		// This continuation will be executed by thread X,
		// and this will have the same execution context as X,
		// which is why the value is 55.
		asyncLocal.Value.Should().Be(55);
	}

	private Task DisposeAsync()
	{
		var asyncFlowControl = ExecutionContext.SuppressFlow();
		
		// Suppressed, so nothing gets captured by the next call.
		return DisposeAsyncCore(asyncFlowControl);
	}

	private async Task DisposeAsyncCore(AsyncFlowControl asyncFlowControl)
	{
		// Now make sure to stop the suppressing here, as we want to normally flow again from here on.
		asyncFlowControl.Dispose();
		
		await DoStuffAsync();
		
		// The current thread is X.
		asyncLocal.Value = 55;
	} // Async method ends here, but the execution context will not be restored, since there was nothing captured to restore it to.

	private async Task DoStuffAsync()
	{
		await Task.Delay(10);
	}
}

@Taudris It would be great if you could pinpoint the place and reason where you think it doesn't work as I have outlined here. :)

@Taudris
Copy link

Taudris commented Sep 17, 2021

The workaround I proposed is extremely difficult to understand the true behavior of. After several hours of my own testing, I still don't. Case in point: the .ConfigureAwait(false) you added to your test in an edit causes the AsyncLocal to return 0, which breaks the test.

Play around with commenting and uncommenting things in the snippet below. (Heads up: it uses xUnit.) The fact that changing what is being awaited anywhere in the logical call stack can break DisposeAsync() is unacceptable IMO; you have no guarantee that some other piece of code you're calling won't later on be changed in a way that causes your code to break, and the problem will be very difficult to diagnose, especially if whoever has to troubleshoot it is someone other than you.

I can only recommend: Don't use this workaround. You should instead change the design so you don't need it.

public class ExecutionContextFlowTests
{
    private readonly AsyncLocal<int> asyncLocal = new();
    private int local;

    private void SetLocal(int value)
    {
        this.asyncLocal.Value = value;
        this.local = value;
    }

    [Fact]
    public async Task TestExecutionFlow()
    {
        this.SetLocal(42);

        Task? awaitable = this.DisposeAsync();

        Assert.Equal(49, this.local); //sanity check

        //commenting and uncommenting this assert changes the behavior
        Assert.Equal(42, this.asyncLocal.Value); //expected 49, but it's still 42. also, merely reading this.asyncLocal.Value can break the other asserts below.

        //await awaitable;
        await awaitable.ConfigureAwait(false);

        Assert.Equal(55, this.local); //sanity check

        //this assert fails on 0 when using .ConfigureAwait(false) and this method does NOT read asyncLocal.Value.
        //this assert fails on 42 when using .ConfigureAwait(false) and this method reads asyncLocal.Value before awaiting awaitable.
        //this assert fails on 42 when DoStuffAsync() calls Task.Yield() and this method reads asyncLocal.Value before awaiting awaitable.
        Assert.Equal(55, this.asyncLocal.Value);

        //this assert fails when using .ConfigureAwait(false) and this method reads asyncLocal.Value before awaiting awaitable.
        //this assert fails when DoStuffAsync() calls Task.Yield() and this method reads asyncLocal.Value before awaiting awaitable.
        this.RequireEC();
    }

    private Task DisposeAsync()
    {
        AsyncFlowControl asyncFlowControl = ExecutionContext.SuppressFlow();

        this.RequireNoEC();

        return this.DisposeAsyncCore(asyncFlowControl);
    }

    private async Task DisposeAsyncCore(AsyncFlowControl asyncFlowControl)
    {
        this.RequireNoEC();
        this.SetLocal(48); //no effect on asyncLocal
        this.RequireNoEC();

        asyncFlowControl.Dispose();
        this.RequireEC();

        //last change before first await. expected this to be visible to callers prior to awaiting the returned task, but it isn't.
        this.SetLocal(49);

        await this.DoStuffAsync();

        this.SetLocal(55); //this value is visible on asyncLocal to callers after awaiting
    }

    //private async Task DoStuffAsync() => await Task.Delay(10);
    private async Task DoStuffAsync() => await Task.Yield();

    private void RequireEC()
    {
        var ec = ExecutionContext.Capture();
        Assert.NotNull(ec);
        ec!.Dispose();
    }

    private void RequireNoEC()
    {
        var ec = ExecutionContext.Capture();
        Assert.Null(ec);
    }
}

@Taudris
Copy link

Taudris commented Sep 17, 2021

Here's a unit test that demonstrates capturing an ExecutionContext and running code on it. I expect this pattern to be more predictable and less fragile than messing with EC flow.

@meiedomi
Copy link

meiedomi commented Sep 17, 2021

Hey @Taudris, thanks a lot for your efforts. I was able to verify that XUnit messes with the execution context while the other frameworks like NUnit and MSTest don't. A normal console app also doesn't exhibit faulty behavior. So my example with ConfigureAwait(false) works everywhere except within XUnit. And I absolutely have no clue why and I probably don't want to know either. ;)
You can verify that for yourself using my repo: https://github.com/meiedomi/AsyncLocalTesting

I also studied your latest gist. But I can't quite see the behavior I want: Remember that we simulate the TransactionScope with our AsyncLocal value. In your code, setting the value to 48 represents entering a new transaction scope. Then, in the DisposeAsync you set it to 55, which represents a potential parent transaction scope. However, the code that actually runs inside of the unit of work must see 48 as well, since we want to issue SQL in the unit of works transaction scope.
So the value on this line should be 48, and not 42. Or am I confusing something myself here?

Don't use this workaround

I agree with this. To be honest, at work I already refactored to the solution with the higher order function I proposed earlier. It doesn't mess with the execution context at all and gives easy and predictable behavior.

I still find this discussion very interesting, and I think it also should make clear that an IAsyncDisposable transaction scope will probably never happen. For good. :)

@Taudris
Copy link

Taudris commented Sep 18, 2021

The NUnit and MSTest tests still break when switching out the Task.Delay(10) for Task.Yield(). But if you have both Delay and Yield in any order, it works. The console output from the AsyncLocal change logging (great idea btw) is quite a bit different as well. So yeah, even with xUnit having its own problems, the workaround is still pretty fragile.

This line actually should not see the new value. The idea is for UnitOfWork to capture and maintain its own ExecutionContext and never expose it to callers. So TestCapturedExecutionContext() only ever sees its own 42, and so would not be able to participate in UnitOfWork's transaction. (It could create its own TransactionScope, which UnitOfWork would see in its constructor and be able to associate with it. But no other UnitOfWork methods would be able to associate with callers' TransactionScopes; just the constructor where the initial capture happens.)

@meiedomi
Copy link

meiedomi commented Sep 18, 2021

Alright, this scares the hell out of me. I stop doing further investigations on your original workaround solution and conclude that we just shouldn't mess with suppressing the execution context across async boundaries. I'm out of mental models to explain the observed behavior.

Thanks also for clarifying your design intent for your latest try. This then means that anything running inside the unit of work that wants to inherit the transaction scope (not the other way around as you explained) would need to go through some public method of the unit of work such that it can establish its internally maintained execution context. Nesting another unit of work inside of it by just using it would not capture the transaction scope. Also, when establishing a log context for example (or any other custom AsyncLocals for that matter), it won't be observed inside of the unit of work, since it uses its own execution context. Certainly this is better and more stable than the flaky workaround though. :)

@Timovzl
Copy link
Author

Timovzl commented Sep 24, 2021

Here is another thought: Would it help if, after doing our own disposal work, we called TransactionScope.Dispose() from a ContinueWith()?

This avoids the async keyword and gives us control over which Task the continuation is attached to. It might offer a way.

The thing is, ContinueWith() still confuses me. You want get it absolutely right, as if you had first awaited the prior work within a try block and then called TransactionScope.Dispose() from the finally block. I'm not entirely sure how to achieve precisely that effect in all situations.

@meiedomi
Copy link

meiedomi commented Sep 30, 2021

@Timovzl I think that ContinueWith inherits the execution context from the thread calling that method, and not from the task that eventually kicks off the continuation. But either way, the problem is that the mere act of inheriting the execution context is enough to lose the original context in which the unit of work is created and run, and thus the TransactionScope will not be properly restored.

I guess that IAsyncDisposable simply does not play well with any type of ambient state management, which TransactionScope is just one example of. As far as we are concerned, switching to a functional approach got rid of the issue. For the future, I noted that Dispose/AsyncDispose should really only be used for what it is designed for: Cleanup of resources. Not anything else. ;)

@Timovzl
Copy link
Author

Timovzl commented Sep 30, 2021

@meiedomi Actually, it just hit me that the problem is that the deeper async layers cannot make changes that are observable to the outer layers. We can circumvent this with a simple layer of indirection.

As a crude first attempt:

  • Use an AsyncLocal<UnitOfWork[]>. (Yes, an array. Eventually we might use a more efficient wrapper, but a single-element array serves our purposes for now.)
  • To set the AsyncLocal<T> value:
    • If Value is null, set Value = new UnitOfWork[1].
    • Assign the intended value to Value[0].
  • To get the AsyncLocal<T> value:
    • Get Value?[0].

The calling code has assigned the array, and populated element 0. DisposeAsync, being deeper down, can see it. It then modifies the array by nulling out element 0. Later, back up the chain, the calling code observes the modification to "its" array.

@meiedomi
Copy link

meiedomi commented Oct 1, 2021

@Timovzl I had the very same idea pretty early on. Why not just put the value on the heap (i.e. array/box/whatever) and therefore share it among any async flow? :) And this would actually work for our own unit of work stuff!

However, the issue is that I still need to dispose the TransactionScope. And the implementation thereof uses its own AsyncLocal to manage the ambient transaction, and doesn't use a box for it. The only thing that would make TransactionScope work is to make sure we dispose it in the same async layer (I like that term ;)) where we created it, which puts us back to square one, unfortunately.

@Timovzl
Copy link
Author

Timovzl commented Oct 1, 2021

@meiedomi Argh! 😛 It truly is a tricky one when we have no control over the type working with AsyncLocal<T>.

In any case, at least we have a solution for the changes to TransactionScope itself that this issue is about. @HongGit @StephenMolloy

@AntonC9018
Copy link

When can we expect this to get fixed?

@ProTip
Copy link

ProTip commented Jun 10, 2023

Lack of async API on TransactionScope is making it unusable in 2023. Projects like Quartz.Net and Npgsql itself would benefit from this so there is an "official" transaction scope API that will allow separate projects/libraries to participate in the same transaction..

As it is bespoke APIs need to be created. I believe there was even a request on the Npgsql project to add some flag on the connection that can be used to determine if a transaction has been opened yet or not..

@StephenMolloy
Copy link
Member

Responsibility for the Transactions area migrated to another team some time ago. I haven't seen them chime in on this thread, so I don't know if this issue is on their radar. @roji?

@roji
Copy link
Member

roji commented Jun 12, 2023

@StephenMolloy @ProTip this issue is on my radar, but this unfortunately involves far more than simply making TransactionScope itself IAsyncDisposable. When only one connection (AKA resource) is enlisted to the transaction (non-distributed), disposing the TransactionScope ends up directly calling commit/rollback on e.g. the enlisted ADO.NET. The APIs there (i.e. IPromotableSinglePhaseNotification) would also need to be retrofitted with async APIs, and all database drivers (and anything else that supports System.Transactions) would need to react to that, implementing async versions of commit/rollback.

This is unfortunately quite a big change, which would require both careful designing and also propagating of the new functionality across the ecosystem (e.g. SqlClient, Npgsql...). As of now, the 23 votes on this issue don't indicate a huge interest in this, so there's little chance we'll prioritize this work in the near future... If asynchronous commit/rollback is important to you, you'll have to stick to DbTransaction for now (note that SqlClient hasn't yet implemented async there either).

Note that when the transaction is distributed, System.Transactions communicates with MSDTC instead. This process should also ideally be asynchronous, which is yet another complication in this whole thing.

@ProTip
Copy link

ProTip commented Jul 26, 2023

It's just kind of a shame since full async coverage seems to be so important to the ecosystem and the Asp.Net Core project itself as it warns in their best practices to Avoid blocking calls.

Would it be a viable path forward to create an AsyncTransactionScope that could slowly phase out TransactionScope, or at least libraries could adopt compatibility of at their own pace? I believe this was the approach taken with IAsyncDisposable and AsyncServiceScope.

@roji roji changed the title IAsyncDisposable TransactionScope Add async support to System.Transactions (e.g. TransactionScope) Jul 26, 2023
@roji
Copy link
Member

roji commented Jul 26, 2023

@ProTip a separate AsyncTransactionScope on its own wouldn't help, not any more than simply making TransactionScope implement IAsyncDisposable: the problem is in the implementation behind that. As I wrote above, the problem is in the interactions between System.Transactions (which is behind TransactionScope) and the specific database drivers (like SqlClient or Npgsql); those APIs also have to be retrofitted with async method counterparts, and the different drivers have to be updated to use them as well.

Unfortunately there's simply no quick and easy fix here - for this to work, async would need to be properly plumbed through System.Transactions, and then to the drivers.

@ProTip
Copy link

ProTip commented Jul 26, 2023

Ah, I thought perhaps there was a way to reduce the up-front burden. Thanks for the detailed response.

@zejji
Copy link

zejji commented Feb 2, 2024

@roji

Is there any news as to whether this is on the roadmap for EF Core 9?

Just about to embark on the implementation of an accounting system (currently in the R&D phase) that will make heavy use of transactions and PostgreSQL's serializable snapshot isolation level and having async support for transaction scopes would be extremely useful.

Or would it be prudent to assume this is unlikely to be implemented and ensure that we fit within the constraints of DbTransaction instead?

@roji
Copy link
Member

roji commented Feb 2, 2024

@zejji this isn't on the roadmap, as you can see by the milestone of the issue; this is a very non-trivial piece of work, and there are generally very few votes. So yes, at this point I'd recommend using DbTransaction in order to get fully async transaction handling.

@zejji
Copy link

zejji commented Feb 2, 2024

@roji Will do - thank you for confirming

@karlra
Copy link

karlra commented Mar 1, 2024

@roji

I think this feature is so important that my company is willing to finance its development. Is there any such program at microsoft?

@roji
Copy link
Member

roji commented Mar 1, 2024

@karlra I'm not aware of anything specific (but that doesn't mean something doesn't exist). One suggestion would be to contact a vendor who would do the work and submit the relevant PR/PRs, which we'd review as usual.

For this particular issue, the change likely is going to be quite complex (which is why it hasn't yet been), and it would also need to span both the runtime and the database driver(s) in question (e.g. SqlClient), which are not part of the runtime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Transactions enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests