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 the ability to change the SQL Write Lock TimeOut #9744

Merged
merged 2 commits into from
Feb 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 23 additions & 0 deletions src/Umbraco.Core/Configuration/GlobalSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -391,5 +391,28 @@ public bool UseHttps
}
}
}


/// <summary>
/// An int value representing the time in milliseconds to lock the database for a write operation
/// </summary>
/// <remarks>
/// The default value is 1800 milliseconds
/// </remarks>
/// <value>The timeout in milliseconds.</value>
public int SqlWriteLockTimeOut
{
get
{
try
{
return int.Parse(ConfigurationManager.AppSettings[Constants.AppSettings.SqlWriteLockTimeOut]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be tryparse and not have a try/catch, it's very easy to do error checking here without any need for try/catch. This should also store the result of this in a backing field so that it's not re-reading from config and re-parsing this value on every single usage of this - which will be a lot because this will be accessed for every read/write lock (lots).

Copy link
Member Author

Choose a reason for hiding this comment

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

I was just repeating the code above! 😅
Good point in adding a backing field here, I was under the assumption these settings were cached somewhere. 😄

}
catch
{
return 1800;
Copy link
Contributor

Choose a reason for hiding this comment

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

Default should be changed to perhaps 5000.

Copy link
Member Author

Choose a reason for hiding this comment

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

I kept it the same so that it will not change any behavior for now unless specifically configured.

}
}
}
}
}
5 changes: 5 additions & 0 deletions src/Umbraco.Core/Configuration/IGlobalSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -72,5 +72,10 @@ public interface IGlobalSettings
/// Gets the location of temporary files.
/// </summary>
string LocalTempPath { get; }

/// <summary>
/// Gets the write lock timeout.
/// </summary>
int SqlWriteLockTimeOut { get; }
}
}
8 changes: 8 additions & 0 deletions src/Umbraco.Core/Constants-AppSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,14 @@ public static class Debug
/// </summary>
public const string DatabaseFactoryServerVersion = "Umbraco.Core.Debug.DatabaseFactoryServerVersion";
}

/// <summary>
/// An int value representing the time in milliseconds to lock the database for a write operation
/// </summary>
/// <remarks>
/// The default value is 1800 milliseconds
/// </remarks>
public const string SqlWriteLockTimeOut = "Umbraco.Core.SqlWriteLockTimeOut";
}
}
}
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
using System;
using System.Collections.Generic;
using System.Configuration;
using System.Data;
using System.Data.SqlServerCe;
using System.Linq;
using NPoco;
using Umbraco.Core.Composing;
using Umbraco.Core.Persistence.DatabaseAnnotations;
using Umbraco.Core.Persistence.DatabaseModelDefinitions;

Expand Down Expand Up @@ -163,7 +165,8 @@ public override void WriteLock(IDatabase db, params int[] lockIds)
if (db.Transaction.IsolationLevel < IsolationLevel.RepeatableRead)
throw new InvalidOperationException("A transaction with minimum RepeatableRead isolation level is required.");

db.Execute(@"SET LOCK_TIMEOUT 1800;");
var timeOut = Current.Configs.Global().SqlWriteLockTimeOut;
db.Execute(@"SET LOCK_TIMEOUT " + timeOut + ";");
nul800sebastiaan marked this conversation as resolved.
Show resolved Hide resolved
// *not* using a unique 'WHERE IN' query here because the *order* of lockIds is important to avoid deadlocks
foreach (var lockId in lockIds)
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
using System;
using System.Collections.Generic;
using System.Configuration;
using System.Data;
using System.Data.Common;
using System.Data.SqlClient;
using System.Linq;
using NPoco;
using Umbraco.Core.Composing;
using Umbraco.Core.Logging;
using Umbraco.Core.Persistence.DatabaseModelDefinitions;
using Umbraco.Core.Scoping;
Expand Down Expand Up @@ -254,7 +256,8 @@ public override bool DoesTableExist(IDatabase db, string tableName)

public override void WriteLock(IDatabase db, params int[] lockIds)
{
WriteLock(db, TimeSpan.FromMilliseconds(1800), lockIds);
var timeOut = Current.Configs.Global().SqlWriteLockTimeOut;
WriteLock(db, TimeSpan.FromMilliseconds(timeOut), lockIds);
}

public void WriteLock(IDatabase db, TimeSpan timeout, params int[] lockIds)
Expand Down
3 changes: 2 additions & 1 deletion src/Umbraco.Tests/TestHelpers/SettingsForTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ public static IGlobalSettings GenerateMockGlobalSettings()
settings.LocalTempStorageLocation == LocalTempStorage.Default &&
settings.LocalTempPath == IOHelper.MapPath("~/App_Data/TEMP") &&
settings.ReservedPaths == (GlobalSettings.StaticReservedPaths + "~/umbraco") &&
settings.ReservedUrls == GlobalSettings.StaticReservedUrls);
settings.ReservedUrls == GlobalSettings.StaticReservedUrls &&
settings.SqlWriteLockTimeOut == 1800);
return config;
}

Expand Down