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 TransactionManager.ImplicitDistributedTransactions #76469

Closed
terrajobst opened this issue Sep 30, 2022 · 12 comments · Fixed by #76376 or #76838
Closed

Add TransactionManager.ImplicitDistributedTransactions #76469

terrajobst opened this issue Sep 30, 2022 · 12 comments · Fixed by #76376 or #76838
Labels
api-approved API was approved in API review, it can be implemented area-System.Transactions blocking Marks issues that we want to fast track in order to unblock other important work
Milestone

Comments

@terrajobst
Copy link
Member

terrajobst commented Sep 30, 2022

Background and motivation

Taken from #76376:

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 @roji @ajcvickers

API Proposal

namespace System.Transactions;

public partial class TransactionManager
{
    [SupportedOSPlatform("windows")]
    public static bool ImplicitDistributedTransactions { get; set; }
}

API Usage

static void Main(string[] args)
{
    //...

    // Early on, the app is configured to enlist transactions into DTC:
    TransactionManager.ImplicitDistributedTransactions = true;

    //...
}
@terrajobst terrajobst added area-System.Data api-ready-for-review API is ready for review, it is NOT ready for implementation labels Sep 30, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Sep 30, 2022
@ghost
Copy link

ghost commented Sep 30, 2022

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

Issue Details

Background and motivation

Taken from #76376:

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 @roji @ajcvickers

API Proposal

namespace System.Transactions;

public partial class TransactionManager
{
    public static bool ImplicitDistributedTransactions { get; set; }
}

API Usage

static void Main(string[] args)
{
    //...

    // Early on, the app is configured to enlist transactions into DTC:
	TransactionManager.ImplicitDistributedTransactions = true;

    //...
}

Alternative Designs

No response

Risks

No response

Author: terrajobst
Assignees: -
Labels:

area-System.Data, api-ready-for-review

Milestone: -

@terrajobst terrajobst changed the title [API Proposal]: Add TransactionManager.ImplicitDistributedTransactions Sep 30, 2022
@terrajobst terrajobst removed the untriaged New issue has not been triaged by the area owner label Sep 30, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Sep 30, 2022
@roji roji linked a pull request Sep 30, 2022 that will close this issue
@terrajobst
Copy link
Member Author

My understanding is that DTC is Windows-only tech. Should the property be marked as [SupportedOSPlatform("windows")]? I'd assume setting the API to true is a no-op on non-Windows, so it's not strictly speaking required, but I could see this being confusing when people don't realize that.

Assuming the flag is not initialized I assume the default is false. Is there a way to say for a specific transaction that it should be enlisted into the DTC without setting this global flag?

@ghost
Copy link

ghost commented Sep 30, 2022

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

Issue Details

Background and motivation

Taken from #76376:

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 @roji @ajcvickers

API Proposal

namespace System.Transactions;

public partial class TransactionManager
{
    public static bool ImplicitDistributedTransactions { get; set; }
}

API Usage

static void Main(string[] args)
{
    //...

    // Early on, the app is configured to enlist transactions into DTC:
    TransactionManager.ImplicitDistributedTransactions = true;

    //...
}
Author: terrajobst
Assignees: -
Labels:

area-System.Transactions, api-ready-for-review, in-pr

Milestone: -

@roji
Copy link
Member

roji commented Sep 30, 2022

@terrajobst thanks for creating the issue.

Yeah, the flag is a no op on non-Windows platforms (see here), and yeah, its default is false (uninitialized). Re using the flag on non-Windows platforms, that may be important as cross-platform applications may always set the flag to true (to enable distributed transactions on Windows), but then refrain from actually using them when running on non-Windows (not allowing the flag to be set on non-Windows would create a bit of friction there). I'm not sure what [SupportedOSPlatform] actually does - is it just a declarative way of telling the user the this API is only usable on that platform?

Is there a way to say for a specific transaction that it should be enlisted into the DTC without setting this global flag?

Unfortunately not... System.Transactions was designed very intentionally to not have an explicit gesture/API for starting a distributed transaction; that's the reason many people unintentionally found themselves starting one, and why we're introducing the flag now. It's theoretically possible to add a flag to TransactionScope/CommittableTransaction, but it's quite a larger effort to flow that down etc. The main goal here is to avoid the accidental usage of distributed transactions in applications which never use them, rather than to fix the System.Transactions design issue (with a per-transaction opt-in), so a simple global flag seems sufficient...

Note that in any future, cross-platform distributed transaction API (#71769), the plan would be to make the API very explicit/opt-in to avoid this design issue. So the flag being discussed now wouldn't affect that scenario (and that's why it has "Implicit" in the name).

@terrajobst
Copy link
Member Author

terrajobst commented Sep 30, 2022

I'm not sure what [SupportedOSPlatform] actually does - is it just a declarative way of telling the user the this API is only usable on that platform?

Sort of. Since .NET 5, we have an analyzer that warns when you use a platform-specific API outside of a platform-specific context. That context can either be an if check or marking the calling code with the same attribute (hence forwarding the platform constraint to the caller). The intention of this analyzer is to raise awareness of APIs that might not work at runtime, by, for example, throwing PlatformNotSupportedException. Another option is to suppress the warning. However, for your case the most natural fix would be this:

// Early on, the app is configured to enlist transactions into DTC:
if (OperatingSystem.IsWindows())
{
    TransactionManager.ImplicitDistributedTransactions = true;
}

The analyzer won't warn in this case and the caller doesn't need to use #if. But it's clear to the reader that this code doesn't have any effect on non-Windows. I think I would personally prefer this, but I can also see the argument that the warning is noise. It's trade-off between not surprising people and not causing noise. I'm curious what others think.

Unfortunately not... System.Transactions was designed very intentionally to not have an explicit gesture/API for starting a distributed transaction;

I see. That is unfortunate in that we only have a "big hammer": either everything is enlisted into the DTC or nothing is. Given that the design of System.Transactions everything is always enlisted, the flag is certainly an improvement though, so I don't think "big hammer" is an argument against the introduction of this flag. Just if we were to design it today, we probably shouldn't do it like that.

@roji
Copy link
Member

roji commented Sep 30, 2022

Re [SupportedOSPlatform], thanks for the explanation. Yeah, I can see this making it clearer that the feature is Windows-only when writing the code, as opposed to runtime exceptions (and making that more explicit in the code).

[...] either everything is enlisted into the DTC or nothing is.

Just a correction on this - turning on ImplicitDistributedTransactions doesn't mean that everything is always enlisted in to the DTC. It just means that this will happen if needed, e.g. if the user attempts to enlist two database connections into the same TransactionScope. If only a single (promotable) connection is used with TransactionScope, a distributed transaction doesn't get created regardless of the flag (that's already the case today before my changes in 7.0). So the flag is basically about enabling the creation of distributed transactions for the cases where they're required.

@terrajobst
Copy link
Member Author

So should we apply [SupportedOSPlatform] then?

@roji
Copy link
Member

roji commented Sep 30, 2022

Sure, makes sense to me.

@terrajobst
Copy link
Member Author

Updated

@roji
Copy link
Member

roji commented Oct 1, 2022

@terrajobst small note: we can put the [SupportedOSPlatform("windows")] specifically on the setter, this way an application can check whether the flag is enabled without having to be on Windows etc. Makes sense?

(I pushed this into the PR in #76376)

@teo-tsirpanis
Copy link
Contributor

If this is for 7.0.0, should it be marked as blocking?

@jkotas jkotas added the blocking Marks issues that we want to fast track in order to unblock other important work label Oct 1, 2022
@teo-tsirpanis teo-tsirpanis added this to the 7.0.0 milestone Oct 1, 2022
@terrajobst
Copy link
Member Author

terrajobst commented Oct 4, 2022

Video

  • @roji suggested to put the attribute on the setter only, which makes sense.
  • We'd prefer a design where the second time the property is set to something else, we throw NotSupportedException
    • Also, System.Transactions should freeze the value once it's read, also causing subsequent sets to throw
namespace System.Transactions;

public partial class TransactionManager
{   
    public static bool ImplicitDistributedTransactions { get; [SupportedOSPlatform("windows")] set; }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Oct 4, 2022
roji added a commit to roji/runtime that referenced this issue Oct 9, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Oct 10, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Oct 10, 2022
roji added a commit to roji/runtime that referenced this issue Oct 10, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Oct 11, 2022
roji added a commit that referenced this issue Oct 11, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Transactions blocking Marks issues that we want to fast track in order to unblock other important work
Projects
None yet
4 participants