Add setter to TransactionManager.DefaultTimeout and MaxTimeout. #71703

imcarolwang · 2022-07-06T09:56:58Z

@HongGit , @mconnew , @stephentoub, this is for #71025 and #59282.

Test scenario:

set DefaultTimeout and MaxTimeout with value in normal/supported range, get expected value being set
set DefaultTimeout with value greater than MaxTimeout, get the MaxTimeout value for DefaultTimeout
set DefaultTimeout and MaxTimeout with negative timespan, throws ArgumentOutOfRangeException

dotnet-issue-labeler · 2022-07-06T09:57:10Z

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.

src/libraries/System.Transactions.Local/src/System/Transactions/TransactionManager.cs

imcarolwang · 2022-07-06T11:10:25Z

src/libraries/System.Transactions.Local/src/System/Transactions/TransactionManager.cs

+                    }
+                }
+
+                s_defaultTimeoutValidated = true;


Should DefaultTimeout always be less than MaxTimeout? If so, when setting either of the two property values, the other property value needs to be validated and adjusted accordingly.

My intuition says yes, default should always be less than max. What did the config system do in 4.8?

It appears that ValidateTimeout() already handles this case here. But a similar validtation would need to be added in the setter for MaxTimeout.

In 4.8 it did one-time loading from config settings and adjust, test result shows default always less than max. I have added ValidateTimeout for default timeout value in the MaxTimeout setter.

StephenMolloy · 2022-07-06T19:18:01Z

src/libraries/System.Transactions.Local/src/System/Transactions/TransactionManager.cs

+                }
+
+                s_cachedMaxTimeout = false;
+                LazyInitializer.EnsureInitialized(ref s_maximumTimeout, ref s_cachedMaxTimeout, ref s_classSyncObject, () => value);


Is there value to using the LazyInitializer pattern here? We already have the value. It's just a simple assignment at this point. Why don't we set s_cachedMaxTimeout to true and just assign the value here?

I was thinking the same thing. The LazyInitializer handles lazily assigning an object to s_classSyncObject for locking so that might be null. I don't see any reason to need a lock here though.

Thanks for the explanation! It's updated to set s_cachedMaxTimeout to true and assign the value directly.

src/libraries/System.Transactions.Local/src/System/Transactions/TransactionManager.cs

stephentoub · 2022-07-08T00:27:35Z

Thanks for your interest in contributing here. The API in #59282 hasn't been approved yet. We generally discourage PRs for new APIs that haven't yet been approved, but regardless we shouldn't merge this until it is.

mconnew · 2022-07-08T19:49:14Z

src/libraries/System.Transactions.Local/src/System/Transactions/TransactionManager.cs

+                s_cachedMaxTimeout = true;
+                s_maximumTimeout = value;
+                s_defaultTimeout = ValidateTimeout(s_defaultTimeout);
+                s_defaultTimeoutValidated = true;


We should log the same event if the default timeout gets reduced. Save a local copy of s_defaultTimeout before the call to ValidateTimeout, then do a comparison and emit an event similar to the DefaultTimeout setter.

Thanks Matt! Updated.

imcarolwang · 2022-07-12T02:36:59Z

src/libraries/System.Transactions.Local/tests/TransactionManagerTest.cs

+            TimeSpan tsMax = TimeSpan.Parse("00:30:00");
+
+            TransactionManager.DefaultTimeout = tsDefault;
+            Assert.Equal(tsDefault, TransactionManager.DefaultTimeout);


In the pipeline run, test sometimes fails (expected 00:02:00, actual 00:01:00) at this line on mono platform. I think the error reproduces more than 80% of the time, I initially thought running the cases in parallel caused the problem, but after writing all the settings in one method, the problem persists. I have no idea how this happened, @StephenMolloy, @mconnew, any thoughts?

mconnew · 2022-07-13T19:00:27Z

@imcarolwang, I just reread the original API proposal and there was a comment by Levi pointing out the potential for data tearing problems in the implementation and to be aware of it. Basically a long is 64bits and on a 32bit platform the two halves are set in two different machine instructions. If you have two different threads setting 2 different values at the same time and you are running on a 32bit platform, you can get the low 32bits from Thread 1 and the High 32bits from Thread 2 and end up with a value which is different from what either thread is trying to set.
There are a few possible solutions. One is to use the LazyInitializer, but I think that probably won't work after setting it the first time. The second is to set it using Interlocked.Exchange. The third is to set it inside of a lock statement. I think the interlocked solution is probably the best as you don't need to worry about lazily initializing the lock, and on 64bit platforms it gets converted by the jit to a simple memory store as that's atomic on 64bit platforms.

imcarolwang · 2022-07-14T04:15:41Z

Thank you @mconnew for the comments! This morning I tried the Interlocked.Exchange approach in a different test PR, the same failure happened on "mono OSX x64 Debug" (for current PR it failed initially and passed in a latter re-ran). As Interlocked.Exchange doesn't have overload with TimeSpan so I convert it to ticks with type long, does it look right to you? I was thinking that if the data tearing thing doesn't happen on x64 platform, then the failure is caused by something else.

mconnew · 2022-08-01T21:09:50Z

src/libraries/System.Transactions.Local/src/System/Transactions/TransactionManager.cs

+
+                long defaultTimeoutTicks = Interlocked.Read(ref s_defaultTimeoutTicks);
+                Interlocked.Exchange(ref s_defaultTimeoutTicks, ValidateTimeout(new TimeSpan(Interlocked.Read(ref s_defaultTimeoutTicks))).Ticks);
+                if (Interlocked.Read(ref s_defaultTimeoutTicks) != defaultTimeoutTicks)


You are using Interlocked.Read to read s_defaultTimeoutTicks, but you just did this on the previous line. You can reuse the local defaultTimeoutTicks here instead of doing a second Interlocked.Read

mconnew · 2022-08-01T21:12:45Z

@imcarolwang, once you remove the additional Interlocked, this is good to check in.

ghost assigned imcarolwang Jul 6, 2022

dotnet-issue-labeler bot added area-System.Transactions new-api-needs-documentation labels Jul 6, 2022

imcarolwang commented Jul 6, 2022

View reviewed changes

src/libraries/System.Transactions.Local/src/System/Transactions/TransactionManager.cs Show resolved Hide resolved

imcarolwang commented Jul 6, 2022

View reviewed changes

HongGit requested review from HongGit, StephenMolloy and mconnew July 6, 2022 18:27

StephenMolloy reviewed Jul 6, 2022

View reviewed changes

mconnew reviewed Jul 6, 2022

View reviewed changes

src/libraries/System.Transactions.Local/src/System/Transactions/TransactionManager.cs Show resolved Hide resolved

HongGit approved these changes Jul 8, 2022

View reviewed changes

mconnew reviewed Jul 8, 2022

View reviewed changes

imcarolwang force-pushed the issue71025 branch from f924352 to 0ab4b46 Compare July 11, 2022 09:57

imcarolwang commented Jul 12, 2022

View reviewed changes

runfoapp bot mentioned this pull request Jul 12, 2022

System.Runtime.InteropServices.JavaScript.Tests.JSImportExportTest.JSImportObject #71990

Closed

imcarolwang added 8 commits July 22, 2022 01:47

Add setter to TransactionManager.DefaultTimeout and MaxTimeout.

28408bb

Address reveiw feedback.

9028801

Log event if DefaultTimeout get reduced by setting MaxTimeout.

f1df0b9

Update tests.

b375528

Fix test.

6f37aff

Update.

46d25c8

Format and re-org test.

5a9e2ac

Interlocked access timespan to avoid data tear on x86 machine.

9522b8c

imcarolwang force-pushed the issue71025 branch from bdec5c7 to 9522b8c Compare July 22, 2022 09:21

update.

dcfdd51

mconnew reviewed Aug 1, 2022

View reviewed changes

Remove redundant Interlocked.Read.

5a8e030

imcarolwang merged commit 8fc905e into dotnet:main Aug 2, 2022

This was referenced Aug 12, 2022

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

Closed

No way to set TransactionManager.DefaultTimeout #71025

Closed

ghost locked as resolved and limited conversation to collaborators Sep 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add setter to TransactionManager.DefaultTimeout and MaxTimeout. #71703

Add setter to TransactionManager.DefaultTimeout and MaxTimeout. #71703

imcarolwang commented Jul 6, 2022 •

edited

Loading

dotnet-issue-labeler bot commented Jul 6, 2022

imcarolwang Jul 6, 2022

StephenMolloy Jul 6, 2022

imcarolwang Jul 7, 2022

StephenMolloy Jul 6, 2022

mconnew Jul 7, 2022

imcarolwang Jul 7, 2022

stephentoub commented Jul 8, 2022

mconnew Jul 8, 2022

imcarolwang Jul 11, 2022

imcarolwang Jul 12, 2022

mconnew commented Jul 13, 2022

imcarolwang commented Jul 14, 2022 •

edited

Loading

mconnew Aug 1, 2022

mconnew commented Aug 1, 2022

Add setter to TransactionManager.DefaultTimeout and MaxTimeout. #71703

Add setter to TransactionManager.DefaultTimeout and MaxTimeout. #71703

Conversation

imcarolwang commented Jul 6, 2022 • edited Loading

dotnet-issue-labeler bot commented Jul 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub commented Jul 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mconnew commented Jul 13, 2022

imcarolwang commented Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

mconnew commented Aug 1, 2022

imcarolwang commented Jul 6, 2022 •

edited

Loading

imcarolwang commented Jul 14, 2022 •

edited

Loading