-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Comments
@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. |
Based on the votes on this Stackoverflow question (of mine) about that, it looks to me this feature is awaited by many people. |
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, 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. |
@HongGit and @StephenMolloy who own this feature area. |
In our project we wrap the As far as I can tell, there is no way to get around this. Probably the main difficulty for supporting |
@meiedomi Thanks for your addition. Could you elaborate on the issue you're having? In my experience, the following holds true: An In other words:
(As a side note, remember that we can have 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 |
@Timovzl The scope of The current 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 The current implementation of The only way out for us is to not use 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.
}) |
@meiedomi Thanks for the thorough explanation! I might have a solution:
The crux is that the calling method, i.e. the one holding 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 Please report back and share if that solves your issue! |
@Timovzl Thanks a lot for your great solution. I also learned something about Unfortunately, I misrepresented our own problem. :( The stuff we do inside Still more ideas? :) |
This may work:
I don't know which method must contain the call to |
@Taudris Wow that's pretty black magic right there! But it actually works! Thank you! :) I wonder how I could make use of I have a minor but important addition to your solution. If I don't use The reason why my tests were all successful even without the extra This solution also slightly complicates the usage of my unit of work, as I now have to return a 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 |
It seems that I misunderstand |
Well, it turns out that Here's another approach I came up with using At this point, I don't think a perfect solution with your desired constraints is possible. The correct solution is for the creator of |
@meiedomi As per the docs, it is Regardless, I do not believe that
Can you confirm that the boolean passed to |
@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? |
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 Since we want that thread to have the same context as the one we got right after the last 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. :) |
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 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 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);
}
} |
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. |
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 I also studied your latest gist. But I can't quite see the behavior I want: Remember that we simulate the
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 |
The NUnit and MSTest tests still break when switching out the This line actually should not see the new value. The idea is for |
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 |
Here is another thought: Would it help if, after doing our own disposal work, we called This avoids the The thing is, |
@Timovzl I think that I guess that |
@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:
The calling code has assigned the array, and populated element 0. |
@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 |
@meiedomi Argh! 😛 It truly is a tricky one when we have no control over the type working with In any case, at least we have a solution for the changes to |
When can we expect this to get fixed? |
Lack of async API on 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.. |
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? |
@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. |
It's just kind of a shame since full async coverage seems to be so important to the ecosystem and the Would it be a viable path forward to create an |
@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. |
Ah, I thought perhaps there was a way to reduce the up-front burden. Thanks for the detailed response. |
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? |
@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. |
@roji Will do - thank you for confirming |
I think this feature is so important that my company is willing to finance its development. Is there any such program at microsoft? |
@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. |
TransactionScope
is a disposable type that almost always has I/O work to do on disposal.Are there any plans for making
TransactionScope
implementIAsyncDisposable
? (I am not aware of how readily the ADO.NET methods involved are available as async.)The text was updated successfully, but these errors were encountered: