Require global opt-in for distributed transactions

[roji]

This adds a static, global TransactionManager.ImplicitDistributedTransactions flag, which must be explicitly turned on in order for distributed transactions to be supported.

The reasoning here, as advocated by @ajcvickers, is that back in .NET Framework many users were unintentionally causing escalation to a distributed transaction through improper use of TransactionScope, etc. This flag protects users interested in using TransactionScope but who do not want distributed transactions; this is important as distributed transactions shouldn't be used lightly and without careful consideration (dependency on MSDTC - an external component, considerable perf impact, portability to non-Windows platforms...).

Re the flag itself, TransactionManager already has global settings managing transactions. Re the name ImplicitDistributedTransactions, there's a possibility we'd introduce cross-platform distributed transactions into System.Transactions (#71769); if that happens, users will very explicitly create distributed transactions, and would not need to set this flag (hence "Implicit").

/cc @ajcvickers

Closes #76469

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Tagging subscribers to this area: @roji
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This adds a static, global TransactionManager.ImplicitDistributedTransactions flag, which must be explicitly turned on in order for distributed transactions to be supported.

The reasoning here, as advocated by @ajcvickers, is that back in .NET Framework many users were unintentionally causing escalation to a distributed transaction through improper use of TransactionScope, etc. This flag protects users interested in using TransactionScope but who do not want distributed transactions; this is important as distributed transactions shouldn't be used lightly and without careful consideration (dependency on MSDTC - an external component, considerable perf impact, portability to non-Windows platforms...).

Re the flag itself, TransactionManager already has global settings managing transactions. Re the name ImplicitDistributedTransactions, there's a possibility we'd introduce cross-platform distributed transactions into System.Transactions (#71769); if that happens, users will very explicitly create distributed transactions, and would not need to set this flag (hence "Implicit").

/cc @ajcvickers

Author:	roji
Assignees:	-
Labels:	area-System.Transactions, new-api-needs-documentation
Milestone:	-

[stephentoub]

Is the intent here to backport this to release/7.0?

[roji]

Is the intent here to backport this to release/7.0?

Yeah, this is something @ajcvickers and I just discussed, despite it being quite late. Not doing this in 7.0 would make it a breaking change later - @ajcvickers will be bringing this to tactics ASAP.

[jkotas]

Can we also add this as a trimming feature flag that is off by default to allow trimming the distributed transactions code?

https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/trimming-options?pivots=dotnet-7-0#trimming-framework-library-features

[ghost]

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This adds a static, global TransactionManager.ImplicitDistributedTransactions flag, which must be explicitly turned on in order for distributed transactions to be supported.

The reasoning here, as advocated by @ajcvickers, is that back in .NET Framework many users were unintentionally causing escalation to a distributed transaction through improper use of TransactionScope, etc. This flag protects users interested in using TransactionScope but who do not want distributed transactions; this is important as distributed transactions shouldn't be used lightly and without careful consideration (dependency on MSDTC - an external component, considerable perf impact, portability to non-Windows platforms...).

Re the flag itself, TransactionManager already has global settings managing transactions. Re the name ImplicitDistributedTransactions, there's a possibility we'd introduce cross-platform distributed transactions into System.Transactions (#71769); if that happens, users will very explicitly create distributed transactions, and would not need to set this flag (hence "Implicit").

/cc @ajcvickers

Author:	roji
Assignees:	roji
Labels:	area-System.Transactions, new-api-needs-documentation, linkable-framework
Milestone:	-

[carlossanlop]

@roji this was approved by Tactics. Pleased ping me when the unresolved of the feedback is addressed so I can merge it.

[teo-tsirpanis]

roji#3 will solve the trimming issues without adding an ILLink feature switch.

[carlossanlop]

roji#3 will solve the trimming issues without adding an ILLink feature switch.

I see that's a PR in @roji 's runtime fork. Does that mean I should wait for those commits to get merged into this PR, or do you plan on submitting that PR later as a separate 7.0 backport, making this ready to merge?

[roji]

I've tried to do a manual test that the code gets trimmed, and I can't see it happening. I did this by having a minimal console program with a csproj to my locally-build System.Transactions.Local.dll with this PR's changes; running the application after publishing throws Implicit distributed transactions have not been enabled, which seems to prove that the DLL is getting picked up correctly.

Without setting ImplicitDistributedTransactions and publishing trimmed, System.Transaction.Local.dll is 226,304.
When setting the flag to true (and suppressing the [RequiresUnrefrerencedCode] warning it generates), System.Transaction.Local.dll is 228,864.

Inspecting the System.Transactions.Local.dll with DotPeek seems to show all the distributed transaction types - including DtcTransactionConnector - although these should be trimmed.

/cc @teo-tsirpanis @eerhardt

Console program

// #pragma warning disable IL2026
// TransactionManager.ImplicitDistributedTransactions = true;
// #pragma warning restore IL2026

Console.WriteLine(typeof(TransactionScope).Assembly.Location);

await using var conn1 = new SqlConnection("Server=localhost;Database=test;User=SA;Password=Abcd5678;Connect Timeout=60;ConnectRetryCount=0;Encrypt=false");
await using var conn2 = new SqlConnection("Server=localhost;Database=test;User=SA;Password=Abcd5678;Connect Timeout=60;ConnectRetryCount=0;Encrypt=false");

using (var scope = new TransactionScope())
{
    conn1.Open();
    conn2.Open();
}

csproj:

<Project Sdk="Microsoft.NET.Sdk">

    <PropertyGroup>
        <OutputType>Exe</OutputType>
        <TargetFramework>net7.0-windows</TargetFramework>
        <LangVersion>latest</LangVersion>
        <Nullable>enable</Nullable>
        <TreatWarningsAsErrors>false</TreatWarningsAsErrors>
        <PublishTrimmed>true</PublishTrimmed>
        <BuiltInComInteropSupport>true</BuiltInComInteropSupport>
    </PropertyGroup>

    <ItemGroup>
        <PackageReference Include="Microsoft.Data.SqlClient" Version="3.0.1" />
    </ItemGroup>

    <ItemGroup>
        <Reference Include="C:\projects\runtime\artifacts\bin\System.Transactions.Local\Debug\net7.0-windows\System.Transactions.Local.dll" />
    </ItemGroup>

</Project>

[teo-tsirpanis]

You should check which type references DtcTransactionConnector. ILSpy has an "Analyze" option that will help you quickly find out.

[stephentoub]

@teo-tsirpanis, when you prototyped it, did you validate everything was getting trimmed away?

[teo-tsirpanis]

Unfortunately not. 😢 I thought it would be OK since trimming warnings were unsuppressed and did not show up.

[stephentoub]

Yeah, I don't see how this construction would enable trimming.

EDIT: I see now that the object which is the choke point is only allocated from the static property setter, in which case I see how that would work as long as that property setter is never used.

[teo-tsirpanis]

I explained how (I thought) it works in roji#3 (comment). Was any of my assumptions wrong?

[teo-tsirpanis]

I wrote a tiny program that follows the same logic and it works.

using ClassLibrary1;

namespace TrimTest
{
    internal static class Program
    {
        static void Main()
        {
            Console.WriteLine("Hello, World!");
            Call(null);
        }

        static void Call(I? i) => i?.F();
    }
}

In another library

namespace ClassLibrary1
{
    public interface I { void F(); }

    public class C : I { public void F() { } }
}

C did not get created and got trimmed as expected. Similarly if DtcTransactionConnector does not get created (if we don't set the switch to true), it would have not been created and should have been trimmed. Unless something else keeps it. 🤔

@roji if you can send the published trimmed app I will take a look.

[roji]

@teo-tsirpanis sent you an email offline, thanks!

Also squashed and rebased over latest release/7.0.

[teo-tsirpanis]

Thanks for the email @roji. I inspected the trimmed assemblies and DtcTransactionConnector is not there. Nor is TransactionManager.ImplicitDistributedTransactions. Perhaps DotPeek decided to show you a different assembly (I almost never use it, ILSpy is much better).

The trim-unsafe code, the P/Invoke that does the COM interop is removed. Removing more code will need additional refactorings.

[roji]

@teo-tsirpanis thanks, yeah - I mistakenly looked at ITransactionConnector instead of DtcTransactionConnector, I can indeed see the latter getting trimmed (and thanks for the ILSpy tip, it's indeed very useful also for understanding who's referencing a type, keeping it from getting trimmed).

However, as the actual assembly sizes show (2.5k diff), this change alone doesn't allow any significant trimming as there are other references. In order to get real benefits here, it seems that all the trimmable code would have to be refactored behind ITransactionConnector. As we're snapping for 7.0 GA on Monday, I've opened #76791 to track doing this for 8.0.

@carlossanlop this is now ready to merge as-is.

[roji]

/azp run

[azure-pipelines]

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

[carlossanlop]

@carlossanlop this is now ready to merge as-is.

Thanks for letting me know.

I saw you tried to re-run the CI but the command was incomplete. I manually re-ran the single failed leg and will merge once it finishes (assuming none of the failures look related to this change).

[carlossanlop]

CI re-run passed, all green. Approved and signed-off. Ready to merge.

[roji]

@carlossanlop the PR adds XML docs to the new API (ImplicitDistributedTransactions), but do I need to update the XMLs manually in https://github.com/dotnet/dotnet-api-docs?

[carlossanlop]

dotnet-api-docs is the source of truth, so yes, please add it there too.

[roji]

OK. Does the XML need to be generated first with the placeholders, or do I just creat something myself? (sorry, I really don't know much about how this all works)

[carlossanlop]

You can port the triple slash docs automatically to dotnet-api-docs using the PortToDocs.exe tool. You can get it from the https://github.com/dotnet/api-docs-sync repo, and you can install it using the install-as-tool.ps1 script located in the root of the repo.

You would use a command like this:

PortToDocs.exe -Docs %RepoRoot%\dotnet-api-docs\xml -IntelliSense %RepoRoot%\runtime\artifacts\bin -IncludedAssemblies "YourAssemblyHere" -IncludedNamespaces "YourNamespaceHere" -Save true