-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Require global opt-in for distributed transactions #76376
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,8 +4,12 @@ | |
using System.Collections; | ||
using System.Diagnostics.CodeAnalysis; | ||
using System.IO; | ||
using System.Runtime.Versioning; | ||
using System.Threading; | ||
using System.Transactions.Configuration; | ||
#if WINDOWS | ||
using System.Transactions.DtcProxyShim; | ||
#endif | ||
using System.Transactions.Oletx; | ||
|
||
namespace System.Transactions | ||
|
@@ -29,6 +33,10 @@ public static class TransactionManager | |
private static TransactionTable? s_transactionTable; | ||
|
||
private static TransactionStartedEventHandler? s_distributedTransactionStartedDelegate; | ||
|
||
internal const string DistributedTransactionTrimmingWarning = | ||
"Distributed transactions support may not be compatible with trimming. If your program creates a distributed transaction via System.Transactions, the correctness of the application cannot be guaranteed after trimming."; | ||
|
||
public static event TransactionStartedEventHandler? DistributedTransactionStarted | ||
{ | ||
add | ||
|
@@ -391,6 +399,60 @@ public static TimeSpan MaximumTimeout | |
} | ||
} | ||
|
||
/// <summary> | ||
/// Controls whether usage of System.Transactions APIs that require escalation to a distributed transaction will do so; | ||
/// if your application requires distributed transaction, opt into using them by setting this to <see langword="true" />. | ||
/// If set to <see langword="false" /> (the default), escalation to a distributed transaction will throw a <see cref="NotSupportedException" />. | ||
/// </summary> | ||
#if WINDOWS | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Elsewhere in runtime we've used partial files for whole APIs whose implementations differ across platform. If we use ifdefs elsewhere in System.Transactions.Local for this, it's ok. We could also refactor it subsequently if we're trying to get this in asap. |
||
public static bool ImplicitDistributedTransactions | ||
{ | ||
get => DtcProxyShimFactory.s_transactionConnector is not null; | ||
|
||
[SupportedOSPlatform("windows")] | ||
[RequiresUnreferencedCode(DistributedTransactionTrimmingWarning)] | ||
set | ||
{ | ||
lock (s_implicitDistributedTransactionsLock) | ||
{ | ||
// Make sure this flag can only be set once, and that once distributed transactions have been initialized, | ||
// it's frozen. | ||
if (s_implicitDistributedTransactions is null) | ||
{ | ||
s_implicitDistributedTransactions = value; | ||
|
||
if (value) | ||
{ | ||
DtcProxyShimFactory.s_transactionConnector ??= new DtcProxyShimFactory.DtcTransactionConnector(); | ||
} | ||
} | ||
else if (value != s_implicitDistributedTransactions) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this just be EDIT: Looking at the Unix implementation, maybe you've done it this way for consistency with not having to track sets there? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it was explicitly discussed in the API design conversation that repeated setting of the same value should be allowed. I don't have any strong feelings here, but I'm also leaning towards allowing multiple sets for the same value - one could imagine an application with two independent components which attempt to set this to true, and throwing would create some needless friction there. |
||
{ | ||
throw new InvalidOperationException(SR.ImplicitDistributedTransactionsCannotBeChanged); | ||
} | ||
} | ||
stephentoub marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
internal static bool? s_implicitDistributedTransactions; | ||
internal static object s_implicitDistributedTransactionsLock = new(); | ||
#else | ||
public static bool ImplicitDistributedTransactions | ||
{ | ||
get => false; | ||
|
||
[SupportedOSPlatform("windows")] | ||
[RequiresUnreferencedCode(DistributedTransactionTrimmingWarning)] | ||
set | ||
{ | ||
if (value) | ||
{ | ||
throw new PlatformNotSupportedException(SR.DistributedNotSupported); | ||
} | ||
} | ||
} | ||
#endif | ||
|
||
// This routine writes the "header" for the recovery information, based on the | ||
// type of the calling object and its provided parameter collection. This information | ||
// we be read back by the static Reenlist method to create the necessary transaction | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.IO; | ||
using System.Runtime.CompilerServices; | ||
using System.Runtime.InteropServices; | ||
using System.Threading; | ||
using Microsoft.DotNet.RemoteExecutor; | ||
|
@@ -492,6 +493,80 @@ public void GetDtcTransaction() | |
Retry(() => Assert.Equal(TransactionStatus.Committed, tx.TransactionInformation.Status)); | ||
}); | ||
|
||
[ConditionalFact(nameof(IsRemoteExecutorSupportedAndNotNano))] | ||
public void Distributed_transactions_require_ImplicitDistributedTransactions_true() | ||
{ | ||
// Temporarily skip on 32-bit where we have an issue. | ||
if (!Environment.Is64BitProcess) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't that be blocked on 32-bit platforms by modifying the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I admit that between IsRemoteExecutorSupported and the other "skip" stuff in this test suite, I just went for the easy thing here. Yeah, it could probably be cleaned up a bit, but it's probably not super important either. |
||
{ | ||
return; | ||
} | ||
|
||
using var _ = RemoteExecutor.Invoke(() => | ||
{ | ||
Assert.False(TransactionManager.ImplicitDistributedTransactions); | ||
|
||
using var tx = new CommittableTransaction(); | ||
|
||
Assert.Throws<NotSupportedException>(MinimalOleTxScenario); | ||
}); | ||
} | ||
|
||
[ConditionalFact(nameof(IsRemoteExecutorSupportedAndNotNano))] | ||
public void ImplicitDistributedTransactions_cannot_be_changed_after_being_set() | ||
{ | ||
// Temporarily skip on 32-bit where we have an issue. | ||
if (!Environment.Is64BitProcess) | ||
{ | ||
return; | ||
} | ||
|
||
using var _ = RemoteExecutor.Invoke(() => | ||
{ | ||
TransactionManager.ImplicitDistributedTransactions = true; | ||
|
||
Assert.Throws<InvalidOperationException>(() => TransactionManager.ImplicitDistributedTransactions = false); | ||
}); | ||
} | ||
|
||
[ConditionalFact(nameof(IsRemoteExecutorSupportedAndNotNano))] | ||
public void ImplicitDistributedTransactions_cannot_be_changed_after_being_read_as_true() | ||
{ | ||
// Temporarily skip on 32-bit where we have an issue. | ||
if (!Environment.Is64BitProcess) | ||
{ | ||
return; | ||
} | ||
|
||
using var _ = RemoteExecutor.Invoke(() => | ||
{ | ||
TransactionManager.ImplicitDistributedTransactions = true; | ||
|
||
MinimalOleTxScenario(); | ||
|
||
Assert.Throws<InvalidOperationException>(() => TransactionManager.ImplicitDistributedTransactions = false); | ||
TransactionManager.ImplicitDistributedTransactions = true; | ||
}); | ||
} | ||
|
||
[ConditionalFact(nameof(IsRemoteExecutorSupportedAndNotNano))] | ||
public void ImplicitDistributedTransactions_cannot_be_changed_after_being_read_as_false() | ||
{ | ||
// Temporarily skip on 32-bit where we have an issue. | ||
if (!Environment.Is64BitProcess) | ||
{ | ||
return; | ||
} | ||
|
||
using var _ = RemoteExecutor.Invoke(() => | ||
{ | ||
Assert.Throws<NotSupportedException>(MinimalOleTxScenario); | ||
|
||
Assert.Throws<InvalidOperationException>(() => TransactionManager.ImplicitDistributedTransactions = true); | ||
TransactionManager.ImplicitDistributedTransactions = false; | ||
}); | ||
} | ||
|
||
private static void Test(Action action) | ||
{ | ||
// Temporarily skip on 32-bit where we have an issue. | ||
|
@@ -500,6 +575,8 @@ private static void Test(Action action) | |
return; | ||
} | ||
|
||
TransactionManager.ImplicitDistributedTransactions = true; | ||
roji marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// In CI, we sometimes get XACT_E_TMNOTAVAILABLE; when it happens, it's typically on the very first | ||
// attempt to connect to MSDTC (flaky/slow on-demand startup of MSDTC), though not only. | ||
// This catches that error and retries. | ||
|
@@ -549,23 +626,25 @@ private static void Retry(Action action) | |
} | ||
} | ||
|
||
static void MinimalOleTxScenario() | ||
{ | ||
using var tx = new CommittableTransaction(); | ||
|
||
var enlistment1 = new TestEnlistment(Phase1Vote.Prepared, EnlistmentOutcome.Committed); | ||
var enlistment2 = new TestEnlistment(Phase1Vote.Prepared, EnlistmentOutcome.Committed); | ||
|
||
tx.EnlistDurable(Guid.NewGuid(), enlistment1, EnlistmentOptions.None); | ||
tx.EnlistDurable(Guid.NewGuid(), enlistment2, EnlistmentOptions.None); | ||
|
||
tx.Commit(); | ||
} | ||
|
||
public class OleTxFixture | ||
{ | ||
// In CI, we sometimes get XACT_E_TMNOTAVAILABLE on the very first attempt to connect to MSDTC; | ||
// this is likely due to on-demand slow startup of MSDTC. Perform pre-test connecting with retry | ||
// to ensure that MSDTC is properly up when the first test runs. | ||
public OleTxFixture() | ||
=> Test(() => | ||
{ | ||
using var tx = new CommittableTransaction(); | ||
|
||
var enlistment1 = new TestEnlistment(Phase1Vote.Prepared, EnlistmentOutcome.Committed); | ||
var enlistment2 = new TestEnlistment(Phase1Vote.Prepared, EnlistmentOutcome.Committed); | ||
|
||
tx.EnlistDurable(Guid.NewGuid(), enlistment1, EnlistmentOptions.None); | ||
tx.EnlistDurable(Guid.NewGuid(), enlistment2, EnlistmentOptions.None); | ||
|
||
tx.Commit(); | ||
}); | ||
=> Test(MinimalOleTxScenario); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elsewhere I believe we have tests that trimming is working as expected. @eerhardt? Is that something we could add here as well to ensure things we're expecting to be trimmable are in fact trimmable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, the "TrimmingTests" responsibility is to ensure that the functionality still works as expected when trimming is enabled.
We currently only have one test that verifies things are actually trimmed out (because we kept breaking it while refactoring):
runtime/src/libraries/System.Runtime/tests/TrimmingTests/InvariantGlobalizationTrue.cs
Lines 31 to 60 in df26a63