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.

Sign up for GitHub

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

Jump to bottom

Add setter to TransactionManager.DefaultTimeout and MaxTimeout. #71703

Merged
merged 10 commits into from
Aug 2, 2022
Merged

Add setter to TransactionManager.DefaultTimeout and MaxTimeout. #71703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
28408bb
Add setter to TransactionManager.DefaultTimeout and MaxTimeout.
imcarolwang Jul 6, 2022
9028801
Address reveiw feedback.
imcarolwang Jul 7, 2022
f1df0b9
Log event if DefaultTimeout get reduced by setting MaxTimeout.
imcarolwang Jul 11, 2022
b375528
Update tests.
imcarolwang Jul 11, 2022
6f37aff
Fix test.
imcarolwang Jul 11, 2022
46d25c8
Update.
imcarolwang Jul 11, 2022
5a9e2ac
Format and re-org test.
imcarolwang Jul 12, 2022
9522b8c
Interlocked access timespan to avoid data tear on x86 machine.
imcarolwang Jul 22, 2022
dcfdd51
update.
imcarolwang Jul 26, 2022
5a8e030
Remove redundant Interlocked.Read.
imcarolwang Aug 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/libraries/System.Transactions.Local/ref/System.Transactions.Local.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,10 +187,10 @@ public static partial class TransactionInterop
}
public static partial class TransactionManager
{
public static System.TimeSpan DefaultTimeout { get { throw null; } }
public static System.TimeSpan DefaultTimeout { get { throw null; } set { } }
[System.Diagnostics.CodeAnalysis.DisallowNullAttribute]
public static System.Transactions.HostCurrentTransactionCallback? HostCurrentCallback { get { throw null; } set { } }
public static System.TimeSpan MaximumTimeout { get { throw null; } }
public static System.TimeSpan MaximumTimeout { get { throw null; } set { } }
public static event System.Transactions.TransactionStartedEventHandler? DistributedTransactionStarted { add { } remove { } }
public static void RecoveryComplete(System.Guid resourceManagerIdentifier) { }
public static System.Transactions.Enlistment Reenlist(System.Guid resourceManagerIdentifier, byte[] recoveryInformation, System.Transactions.IEnlistmentNotification enlistmentNotification) { throw null; }
Expand Down
68 changes: 61 additions & 7 deletions src/libraries/System.Transactions.Local/src/System/Transactions/TransactionManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ internal static IsolationLevel DefaultIsolationLevel
private static MachineSettingsSection MachineSettings => s_machineSettings ??= MachineSettingsSection.GetSection();

private static bool s_defaultTimeoutValidated;
private static TimeSpan s_defaultTimeout;
private static long s_defaultTimeoutTicks;
public static TimeSpan DefaultTimeout
{
get
Expand All @@ -292,23 +292,45 @@ public static TimeSpan DefaultTimeout

if (!s_defaultTimeoutValidated)
{
s_defaultTimeout = ValidateTimeout(DefaultSettingsSection.Timeout);
// If the timeout value got adjusted, it must have been greater than MaximumTimeout.
if (s_defaultTimeout != DefaultSettingsSection.Timeout)
LazyInitializer.EnsureInitialized(ref s_defaultTimeoutTicks, ref s_defaultTimeoutValidated, ref s_classSyncObject, () => ValidateTimeout(DefaultSettingsSection.Timeout).Ticks);
if (Interlocked.Read(ref s_defaultTimeoutTicks) != DefaultSettingsSection.Timeout.Ticks)
{
if (etwLog.IsEnabled())
{
etwLog.ConfiguredDefaultTimeoutAdjusted();
}
}
s_defaultTimeoutValidated = true;
}

if (etwLog.IsEnabled())
{
etwLog.MethodExit(TraceSourceType.TraceSourceBase, "TransactionManager.get_DefaultTimeout");
}
return s_defaultTimeout;
return new TimeSpan(Interlocked.Read(ref s_defaultTimeoutTicks));
}
set
{
TransactionsEtwProvider etwLog = TransactionsEtwProvider.Log;
if (etwLog.IsEnabled())
{
etwLog.MethodEnter(TraceSourceType.TraceSourceBase, "TransactionManager.set_DefaultTimeout");
}

Interlocked.Exchange(ref s_defaultTimeoutTicks, ValidateTimeout(value).Ticks);
if (Interlocked.Read(ref s_defaultTimeoutTicks) != value.Ticks)
{
imcarolwang marked this conversation as resolved.
Show resolved Hide resolved
if (etwLog.IsEnabled())
{
etwLog.ConfiguredDefaultTimeoutAdjusted();
}
}

s_defaultTimeoutValidated = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (etwLog.IsEnabled())
{
etwLog.MethodExit(TraceSourceType.TraceSourceBase, "TransactionManager.set_DefaultTimeout");
}
}
}

Expand All @@ -325,7 +347,7 @@ public static TimeSpan MaximumTimeout
etwLog.MethodEnter(TraceSourceType.TraceSourceBase, "TransactionManager.get_DefaultMaximumTimeout");
}

LazyInitializer.EnsureInitialized(ref s_maximumTimeout, ref s_cachedMaxTimeout, ref s_classSyncObject, () => DefaultSettingsSection.Timeout);
imcarolwang marked this conversation as resolved.
Show resolved Hide resolved
LazyInitializer.EnsureInitialized(ref s_maximumTimeout, ref s_cachedMaxTimeout, ref s_classSyncObject, () => MachineSettingsSection.MaxTimeout);

if (etwLog.IsEnabled())
{
Expand All @@ -334,6 +356,38 @@ public static TimeSpan MaximumTimeout

return s_maximumTimeout;
}
set
{
TransactionsEtwProvider etwLog = TransactionsEtwProvider.Log;
if (etwLog.IsEnabled())
{
etwLog.MethodEnter(TraceSourceType.TraceSourceBase, "TransactionManager.set_DefaultMaximumTimeout");
}

if (value < TimeSpan.Zero)
{
throw new ArgumentOutOfRangeException(nameof(value));
}

s_cachedMaxTimeout = true;
s_maximumTimeout = value;
LazyInitializer.EnsureInitialized(ref s_defaultTimeoutTicks, ref s_defaultTimeoutValidated, ref s_classSyncObject, () => DefaultSettingsSection.Timeout.Ticks);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
if (etwLog.IsEnabled())
{
etwLog.ConfiguredDefaultTimeoutAdjusted();
}
}

if (etwLog.IsEnabled())
{
etwLog.MethodExit(TraceSourceType.TraceSourceBase, "TransactionManager.set_DefaultMaximumTimeout");
}
}
}

/// <summary>
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Transactions.Local/tests/System.Transactions.Local.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
<Compile Include="NonMsdtcPromoterTests.cs" />
<Compile Include="AsyncTransactionScopeTests.cs" />
<Compile Include="IntResourceManager.cs" />
<Compile Include="TransactionManagerTest.cs" />
<Compile Include="TransactionScopeTest.cs" />
<Compile Include="AsyncTest.cs" />
<Compile Include="EnlistTest.cs" />
Expand Down
31 changes: 31 additions & 0 deletions src/libraries/System.Transactions.Local/tests/TransactionManagerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Xunit;

namespace System.Transactions.Tests
{
public class TransactionManagerTest
{
[Fact]
public void DefaultTimeout_MaxTimeout_Set_Get()
{
TimeSpan tsDefault = TimeSpan.Parse("00:02:00");
TransactionManager.DefaultTimeout = tsDefault;
Assert.Equal(tsDefault, TransactionManager.DefaultTimeout);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


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

TimeSpan ts = TransactionManager.MaximumTimeout.Add(TimeSpan.FromMinutes(10));
TransactionManager.DefaultTimeout = ts;
Assert.Equal(tsMax, TransactionManager.MaximumTimeout);
Assert.Equal(TransactionManager.DefaultTimeout, TransactionManager.MaximumTimeout);

ts = TimeSpan.Parse("-00:01:00");
Assert.Throws<ArgumentOutOfRangeException>(() => TransactionManager.DefaultTimeout = ts);
Assert.Throws<ArgumentOutOfRangeException>(() => TransactionManager.MaximumTimeout = ts);
}
}
}