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

[API Proposal]: add a setter to TransactionManager.DefaultTimeout and MaxTimeout #59282

Closed
wli3 opened this issue Sep 17, 2021 · 11 comments
Closed
Labels
api-approved API was approved in API review, it can be implemented api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Transactions
Milestone

Comments

@wli3
Copy link

wli3 commented Sep 17, 2021

Background and motivation

.NET SDK's workload install feature uses TransactionScope to control rollback. TransactionScope has a machine level timeout of 10 minutes. And that end up causing long workload installation failure. It impacts all slow internet user for .NET 6.0. Currently a workaround is applied dotnet/sdk#21101

Add #71025, customer is asking to add setter for TransactionManager.DefaultTimeout.

API Proposal

namespace System.Transactions
{
    public static class TransactionManager
    {
        public static TimeSpan DefaultTimeout 
        {
            get; 
+          set;
         } 
        public static TimeSpan MaximumTimeout 
        {
            get; 
+          set;
         } 
    }
}

API Usage

In .NET framework, customer could update these settings in machine.config. However, in .NET Core, there is no way to update these settings anymore.
See issue #1418 and #71025, requested by custeomer.

Risks

Low.

@wli3 wli3 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Sep 17, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Transactions untriaged New issue has not been triaged by the area owner labels Sep 17, 2021
@wli3
Copy link
Author

wli3 commented Sep 17, 2021

Which release we want to put this in?

@GrabYourPitchforks
Copy link
Member

Implementation note: make sure the getter and setter are safe for multithreaded access; the data could be torn on 32-bit archs.

@mconnew
Copy link
Member

mconnew commented Sep 18, 2021

@GrabYourPitchforks, Interlocked.Read and Interlocked.Write should do the trick I presume? They become no-ops on 64-bit platforms with a simple 64-bit integer, but I don't know if TimeSpan would be treated the same way and be a no-op.

@wli3, would I be correct in saying it's a bit too tight to get this in to .NET 6? They have a workaround for now, so I think .NET 7 would be sufficient.

@mconnew
Copy link
Member

mconnew commented Sep 18, 2021

Some more thoughts about this, the required value for TransactionManager.MaximumTimeout is mostly affected by the executing environment which is why it was settable via machine.config in NetFx. I think we should also have some non-code way to modify this behavior, e.g. through an AppSetting as I could see a situation where you would want to modify this value for an app in a particular deployed environment and making code changes doesn't seem to be an appropriate remediation when it's not the app that has the problem.

@stephentoub
Copy link
Member

stephentoub commented Sep 18, 2021

They become no-ops on 64-bit platforms with a simple 64-bit integer

On arm, these would still involve fences in the assembly.

(I'm not making any judgements on the rest of the proposal, just clarifying that comment. )

@mconnew
Copy link
Member

mconnew commented Sep 20, 2021

@stephentoub, can data tearing happen on ARM64 with longs?

@stephentoub
Copy link
Member

stephentoub commented Sep 20, 2021

can data tearing happen on ARM64 with longs?

No (at least not in safe code with things properly aligned... I'm not sure about when things aren't aligned naturally)

@StephenMolloy StephenMolloy added this to the 7.0.0 milestone Sep 30, 2021
@StephenMolloy StephenMolloy removed the untriaged New issue has not been triaged by the area owner label Sep 30, 2021
@dsplaisted
Copy link
Member

Looks like this may be a duplicate of #1418

@HongGit HongGit changed the title [API Proposal]: add a setter to TransactionManager.MaximumTimeout [API Proposal]: add a setter to TransactionManager.DefaultTimeout and MaxTimeout Jul 8, 2022
@HongGit HongGit added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work labels Jul 11, 2022
@bartonjs
Copy link
Member

bartonjs commented Jul 14, 2022

Video

  • We would prefer an option without a static setter, like using AppContext to allow the setting to be set (and loaded into a Lazy<T> so it only gets read once)
  • As the next fallback, it might be helpful to have the setter only allow set-once (throw on a second call to set)
namespace System.Transactions
{
    public static class TransactionManager
    {
        public static TimeSpan DefaultTimeout 
        {
            get; 
+           set;
         } 
        public static TimeSpan MaximumTimeout 
        {
            get; 
+           set;
         } 
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 14, 2022
@filipnavara
Copy link
Member

A similar implementation of AppContext switch for timeout is in the Regex class.

@StephenMolloy
Copy link
Member

Fixed by #71703

@ghost ghost locked as resolved and limited conversation to collaborators Sep 12, 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 api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Transactions
Projects
None yet
Development

No branches or pull requests

10 participants