yv01p/Umbraco-CMS
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

U4-11202 - Fix deadlock in UserRepository

Browse Source
This commit is contained in:
Stephan
2018-05-29 18:30:37 +02:00
parent 6208a3ec73
commit 00c62804a8
5 changed files with 248 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files

76
src/Umbraco.Core/Persistence/NPocoSqlExtensions.cs
View File

@@ -916,6 +916,82 @@ namespace Umbraco.Core.Persistence
#endregion
#region Hints
/// <summary>
/// Appends the relevant ForUpdate hint.
/// </summary>
/// <param name="sql">The Sql statement.</param>
/// <returns>The Sql statement.</returns>
public static Sql<ISqlContext> ForUpdate(this Sql<ISqlContext> sql)
{
// MySql wants "FOR UPDATE" at the end, and T-Sql wants "WITH (UPDLOCK)" in the FROM statement,
// and we want to implement it in the least expensive way, so parsing the entire string here is
// a no, so we use reflection to work on the Sql expression before it is built.
// TODO propose a clean way to do that type of thing in NPoco
if (sql.SqlContext.DatabaseType.IsMySql())
{
sql.Append("FOR UDPATE");
return sql;
}
// go find the first FROM clause, and append the lock hint
Sql s = sql;
var updated = false;
while (s != null)
{
var sqlText = SqlInspector.GetSqlText(s);
if (sqlText.StartsWith("FROM ", StringComparison.OrdinalIgnoreCase))
{
SqlInspector.SetSqlText(s, sqlText + " WITH (UPDLOCK)");
updated = true;
break;
}
s = SqlInspector.GetSqlRhs(sql);
}
if (updated)
SqlInspector.Reset(sql);
return sql;
}
#endregion
#region Sql Inspection
private static SqlInspectionUtilities _sqlInspector;
private static SqlInspectionUtilities SqlInspector => _sqlInspector ?? (_sqlInspector = new SqlInspectionUtilities());
private class SqlInspectionUtilities
{
private readonly Func<Sql, string> _getSqlText;
private readonly Action<Sql, string> _setSqlText;
private readonly Func<Sql, Sql> _getSqlRhs;
private readonly Action<Sql, string> _setSqlFinal;
public SqlInspectionUtilities()
{
(_getSqlText, _setSqlText) = ReflectionUtilities.EmitFieldGetterAndSetter<Sql, string>("_sql");
_getSqlRhs = ReflectionUtilities.EmitFieldGetter<Sql, Sql>("_rhs");
_setSqlFinal = ReflectionUtilities.EmitFieldSetter<Sql, string>("_sqlFinal");
}
public string GetSqlText(Sql sql) => _getSqlText(sql);
public void SetSqlText(Sql sql, string sqlText) => _setSqlText(sql, sqlText);
public Sql GetSqlRhs(Sql sql) => _getSqlRhs(sql);
public void Reset(Sql sql) => _setSqlFinal(sql, null);
}
#endregion
#region Utilities
private static string[] GetColumns<TDto>(this Sql<ISqlContext> sql, string tableAlias = null, string referenceName = null, Expression<Func<TDto, object>>[] columnExpressions = null, bool withAlias = true)

42
src/Umbraco.Core/Persistence/Repositories/Implement/UserRepository.cs
View File

@@ -191,7 +191,20 @@ ORDER BY colName";
public bool ValidateLoginSession(int userId, Guid sessionId)
{
var found = Database.FirstOrDefault<UserLoginDto>("WHERE sessionId=@sessionId", new {sessionId = sessionId});
// with RepeatableRead transaction mode, read-then-update operations can
// cause deadlocks, and the ForUpdate() hint is required to tell the database
// to acquire an exclusive lock when reading
// that query is going to run a *lot*, make it a template
var t = SqlContext.Templates.Get("Umbraco.Core.UserRepository.ValidateLoginSession", s => s
.Select<UserLoginDto>()
.From<UserLoginDto>()
.Where<UserLoginDto>(x => x.SessionId == SqlTemplate.Arg<Guid>("sessionId"))
.ForUpdate());
var sql = t.Sql(sessionId);
var found = Database.Query<UserLoginDto>(sql).FirstOrDefault();
if (found == null || found.UserId != userId || found.LoggedOutUtc.HasValue)
return false;
@@ -211,34 +224,21 @@ ORDER BY colName";
public int ClearLoginSessions(int userId)
{
//TODO: I know this doesn't follow the normal repository conventions which would require us to crete a UserSessionRepository
//and also business logic models for these objects but that's just so overkill for what we are doing
//and now that everything is properly in a transaction (Scope) there doesn't seem to be much reason for using that anymore
var count = Database.ExecuteScalar<int>("SELECT COUNT(*) FROM umbracoUserLogin WHERE userId=@userId", new { userId = userId });
Database.Execute("DELETE FROM umbracoUserLogin WHERE userId=@userId", new {userId = userId});
return count;
return Database.Delete<UserLoginDto>(Sql().Where<UserLoginDto>(x => x.UserId == userId));
}
public int ClearLoginSessions(TimeSpan timespan)
{
//TODO: I know this doesn't follow the normal repository conventions which would require us to crete a UserSessionRepository
//and also business logic models for these objects but that's just so overkill for what we are doing
//and now that everything is properly in a transaction (Scope) there doesn't seem to be much reason for using that anymore
var fromDate = DateTime.UtcNow - timespan;
var count = Database.ExecuteScalar<int>("SELECT COUNT(*) FROM umbracoUserLogin WHERE lastValidatedUtc=@fromDate", new { fromDate = fromDate });
Database.Execute("DELETE FROM umbracoUserLogin WHERE lastValidatedUtc=@fromDate", new { fromDate = fromDate });
return count;
return Database.Delete<UserLoginDto>(Sql().Where<UserLoginDto>(x => x.LastValidatedUtc < fromDate));
}
public void ClearLoginSession(Guid sessionId)
{
//TODO: I know this doesn't follow the normal repository conventions which would require us to crete a UserSessionRepository
//and also business logic models for these objects but that's just so overkill for what we are doing
//and now that everything is properly in a transaction (Scope) there doesn't seem to be much reason for using that anymore
Database.Execute("UPDATE umbracoUserLogin SET loggedOutUtc=@now WHERE sessionId=@sessionId",
new { now = DateTime.UtcNow, sessionId = sessionId });
// fixme why is that one updating and not deleting?
Database.Execute(Sql()
.Update<UserLoginDto>(u => u.Set(x => x.LoggedOutUtc, DateTime.UtcNow))
.Where<UserLoginDto>(x => x.SessionId == sessionId));
}
protected override IEnumerable<IUser> PerformGetAll(params int[] ids)
@@ -718,7 +718,7 @@ ORDER BY colName";
if (string.IsNullOrWhiteSpace(orderBy)) throw new ArgumentException("Value cannot be null or whitespace.", nameof(orderBy));
Sql<ISqlContext> filterSql = null;
var customFilterWheres = filter != null ? filter.GetWhereClauses().ToArray() : null;
var customFilterWheres = filter?.GetWhereClauses().ToArray();
var hasCustomFilter = customFilterWheres != null && customFilterWheres.Length > 0;
if (hasCustomFilter
|| includeUserGroups != null && includeUserGroups.Length > 0

110
src/Umbraco.Core/ReflectionUtilities.cs
View File

@@ -21,6 +21,96 @@ namespace Umbraco.Core
/// </remarks>
public static partial class ReflectionUtilities
{
#region Fields
/// <summary>
/// Emits a field getter.
/// </summary>
/// <typeparam name="TDeclaring">The declaring type.</typeparam>
/// <typeparam name="TValue">The field type.</typeparam>
/// <param name="fieldName">The name of the field.</param>
/// <returns>A field getter function.</returns>
/// <exception cref="ArgumentNullOrEmptyException">Occurs when <paramref name="fieldName"/> is null or empty.</exception>
/// <exception cref="InvalidOperationException">Occurs when the field does not exist.</exception>
/// <exception cref="ArgumentException">Occurs when <typeparamref name="TValue"/> does not match the type of the field.</exception>
public static Func<TDeclaring, TValue> EmitFieldGetter<TDeclaring, TValue>(string fieldName)
{
var field = GetField<TDeclaring, TValue>(fieldName);
return EmitFieldGetter<TDeclaring, TValue>(field);
}
/// <summary>
/// Emits a field setter.
/// </summary>
/// <typeparam name="TDeclaring">The declaring type.</typeparam>
/// <typeparam name="TValue">The field type.</typeparam>
/// <param name="fieldName">The name of the field.</param>
/// <returns>A field setter action.</returns>
/// <exception cref="ArgumentNullOrEmptyException">Occurs when <paramref name="fieldName"/> is null or empty.</exception>
/// <exception cref="InvalidOperationException">Occurs when the field does not exist.</exception>
/// <exception cref="ArgumentException">Occurs when <typeparamref name="TValue"/> does not match the type of the field.</exception>
public static Action<TDeclaring, TValue> EmitFieldSetter<TDeclaring, TValue>(string fieldName)
{
var field = GetField<TDeclaring, TValue>(fieldName);
return EmitFieldSetter<TDeclaring, TValue>(field);
}
/// <summary>
/// Emits a field getter and setter.
/// </summary>
/// <typeparam name="TDeclaring">The declaring type.</typeparam>
/// <typeparam name="TValue">The field type.</typeparam>
/// <param name="fieldName">The name of the field.</param>
/// <returns>A field getter and setter functions.</returns>
/// <exception cref="ArgumentNullOrEmptyException">Occurs when <paramref name="fieldName"/> is null or empty.</exception>
/// <exception cref="InvalidOperationException">Occurs when the field does not exist.</exception>
/// <exception cref="ArgumentException">Occurs when <typeparamref name="TValue"/> does not match the type of the field.</exception>
public static (Func<TDeclaring, TValue>, Action<TDeclaring, TValue>) EmitFieldGetterAndSetter<TDeclaring, TValue>(string fieldName)
{
var field = GetField<TDeclaring, TValue>(fieldName);
return (EmitFieldGetter<TDeclaring, TValue>(field), EmitFieldSetter<TDeclaring, TValue>(field));
}
private static FieldInfo GetField<TDeclaring, TValue>(string fieldName)
{
if (string.IsNullOrWhiteSpace(fieldName)) throw new ArgumentNullOrEmptyException(nameof(fieldName));
// get the field
var field = typeof(TDeclaring).GetField(fieldName, BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic);
if (field == null) throw new InvalidOperationException($"Could not find field {typeof(TDeclaring)}.{fieldName}.");
// validate field type
if (field.FieldType != typeof(TValue)) // strict
throw new ArgumentException($"Value type {typeof(TValue)} does not match field {typeof(TDeclaring)}.{fieldName} type {field.FieldType}.");
return field;
}
private static Func<TDeclaring, TValue> EmitFieldGetter<TDeclaring, TValue>(FieldInfo field)
{
// emit
var (dm, ilgen) = CreateIlGenerator(field.DeclaringType?.Module, new [] { typeof(TDeclaring) }, typeof(TValue));
ilgen.Emit(OpCodes.Ldarg_0);
ilgen.Emit(OpCodes.Ldfld, field);
ilgen.Return();
return (Func<TDeclaring, TValue>) (object) dm.CreateDelegate(typeof(Func<TDeclaring, TValue>));
}
private static Action<TDeclaring, TValue> EmitFieldSetter<TDeclaring, TValue>(FieldInfo field)
{
// emit
var (dm, ilgen) = CreateIlGenerator(field.DeclaringType?.Module, new [] { typeof(TDeclaring), typeof(TValue) }, typeof(void));
ilgen.Emit(OpCodes.Ldarg_0);
ilgen.Emit(OpCodes.Ldarg_1);
ilgen.Emit(OpCodes.Stfld, field);
ilgen.Return();
return (Action<TDeclaring, TValue>) (object) dm.CreateDelegate(typeof(Action<TDeclaring, TValue>));
}
#endregion
#region Properties
/// <summary>
@@ -207,7 +297,7 @@ namespace Umbraco.Core
/// is specified and does not match the function's returned type.</exception>
public static TLambda EmitCtor<TLambda>(bool mustExist = true, Type declaring = null)
{
(_, var lambdaParameters, var lambdaReturned) = AnalyzeLambda<TLambda>(true, true);
var (_, lambdaParameters, lambdaReturned) = AnalyzeLambda<TLambda>(true, true);
// determine returned / declaring type
if (declaring == null) declaring = lambdaReturned;
@@ -239,7 +329,7 @@ namespace Umbraco.Core
{
if (ctor == null) throw new ArgumentNullException(nameof(ctor));
(_, var lambdaParameters, var lambdaReturned) = AnalyzeLambda<TLambda>(true, true);
var (_, lambdaParameters, lambdaReturned) = AnalyzeLambda<TLambda>(true, true);
return EmitCtorSafe<TLambda>(lambdaParameters, lambdaReturned, ctor);
}
@@ -281,7 +371,7 @@ namespace Umbraco.Core
{
if (ctor == null) throw new ArgumentNullException(nameof(ctor));
(_, var lambdaParameters, var lambdaReturned) = AnalyzeLambda<TLambda>(true, true);
var (_, lambdaParameters, lambdaReturned) = AnalyzeLambda<TLambda>(true, true);
// emit - unsafe - use lambda's args and assume they are correct
return EmitCtor<TLambda>(lambdaReturned, lambdaParameters, ctor);
@@ -293,7 +383,7 @@ namespace Umbraco.Core
var ctorParameters = GetParameters(ctor);
// emit
(var dm, var ilgen) = CreateIlGenerator(ctor.DeclaringType?.Module, lambdaParameters, declaring);
var (dm, ilgen) = CreateIlGenerator(ctor.DeclaringType?.Module, lambdaParameters, declaring);
EmitLdargs(ilgen, lambdaParameters, ctorParameters);
ilgen.Emit(OpCodes.Newobj, ctor); // ok to just return, it's only objects
ilgen.Return();
@@ -342,7 +432,7 @@ namespace Umbraco.Core
{
if (string.IsNullOrWhiteSpace(methodName)) throw new ArgumentNullOrEmptyException(nameof(methodName));
(var lambdaDeclaring, var lambdaParameters, var lambdaReturned) = AnalyzeLambda<TLambda>(true, out var isFunction);
var (lambdaDeclaring, lambdaParameters, lambdaReturned) = AnalyzeLambda<TLambda>(true, out var isFunction);
// get the method infos
var method = declaring.GetMethod(methodName, BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Static, null, lambdaParameters, null);
@@ -374,7 +464,7 @@ namespace Umbraco.Core
var methodParameters = method.GetParameters().Select(x => x.ParameterType).ToArray();
var isStatic = method.IsStatic;
(var lambdaDeclaring, var lambdaParameters, var lambdaReturned) = AnalyzeLambda<TLambda>(isStatic, out var isFunction);
var (lambdaDeclaring, lambdaParameters, lambdaReturned) = AnalyzeLambda<TLambda>(isStatic, out var isFunction);
// if not static, then the first lambda arg must be the method declaring type
if (!isStatic && (methodDeclaring == null || !methodDeclaring.IsAssignableFrom(lambdaDeclaring)))
@@ -408,7 +498,7 @@ namespace Umbraco.Core
if (method == null) throw new ArgumentNullException(nameof(method));
var isStatic = method.IsStatic;
(var lambdaDeclaring, var lambdaParameters, var lambdaReturned) = AnalyzeLambda<TLambda>(isStatic, out _);
var (lambdaDeclaring, lambdaParameters, lambdaReturned) = AnalyzeLambda<TLambda>(isStatic, out _);
// emit - unsafe - use lambda's args and assume they are correct
return EmitMethod<TLambda>(lambdaDeclaring, lambdaReturned, lambdaParameters, method);
@@ -433,7 +523,7 @@ namespace Umbraco.Core
throw new ArgumentNullOrEmptyException(nameof(methodName));
// validate lambda type
(var lambdaDeclaring, var lambdaParameters, var lambdaReturned) = AnalyzeLambda<TLambda>(false, out var isFunction);
var (lambdaDeclaring, lambdaParameters, lambdaReturned) = AnalyzeLambda<TLambda>(false, out var isFunction);
// get the method infos
var method = lambdaDeclaring.GetMethod(methodName, BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance, null, lambdaParameters, null);
@@ -464,7 +554,7 @@ namespace Umbraco.Core
var methodArgTypes = GetParameters(method, withDeclaring: !method.IsStatic);
// emit IL
(var dm, var ilgen) = CreateIlGenerator(method.DeclaringType?.Module, parameters, lambdaReturned);
var (dm, ilgen) = CreateIlGenerator(method.DeclaringType?.Module, parameters, lambdaReturned);
EmitLdargs(ilgen, parameters, methodArgTypes);
ilgen.CallMethod(method);
EmitOutputAdapter(ilgen, lambdaReturned, method.ReturnType);
@@ -486,7 +576,7 @@ namespace Umbraco.Core
{
var typeLambda = typeof(TLambda);
(var declaring, var parameters, var returned) = AnalyzeLambda<TLambda>(isStatic, out var maybeFunction);
var (declaring, parameters, returned) = AnalyzeLambda<TLambda>(isStatic, out var maybeFunction);
if (isFunction)
{

30
src/Umbraco.Tests/Clr/ReflectionUtilitiesTests.cs
View File

@@ -528,6 +528,26 @@ namespace Umbraco.Tests.Clr
Assert.AreEqual(1, values5D["intValue2"]); // JsonProperty changes property name
}
[Test]
public void EmitFieldGetterSetterEmits()
{
var getter1 = ReflectionUtilities.EmitFieldGetter<Class1, int>("Field1");
var getter2 = ReflectionUtilities.EmitFieldGetter<Class1, int>("Field2");
var c = new Class1();
Assert.AreEqual(33, getter1(c));
Assert.AreEqual(66, getter2(c));
var setter2 = ReflectionUtilities.EmitFieldSetter<Class1, int>("Field2");
setter2(c, 99);
Assert.AreEqual(99, getter2(c));
// works on readonly fields!
var (getter3, setter3) = ReflectionUtilities.EmitFieldGetterAndSetter<Class1, int>("Field3");
Assert.AreEqual(22, getter3(c));
setter3(c, 44);
Assert.AreEqual(44, getter3(c));
}
// fixme - missing tests specifying 'returned' on method, property
#region IL Code
@@ -550,6 +570,12 @@ namespace Umbraco.Tests.Clr
// conv.i4
public void SetIntValue2(Class4 object4, object d) => object4.IntValue = (int) (double) d;
// get field
public int GetIntField(Class1 object1) => object1.Field1;
// set field
public void SetIntField(Class1 object1, int i) => object1.Field1 = i;
#endregion
#region Test Objects
@@ -583,6 +609,10 @@ namespace Umbraco.Tests.Clr
public int Value2 { set { } }
public int Value3 { get { return 42; } set { } }
private int ValueP1 => 42;
public int Field1 = 33;
private int Field2 = 66;
public readonly int Field3 = 22;
}
public class Class2 { }

22
src/Umbraco.Tests/Persistence/NPocoTests/NPocoSqlTests.cs
View File

@@ -1,4 +1,5 @@
using System.Diagnostics;
using System;
using System.Diagnostics;
using NUnit.Framework;
using Umbraco.Core;
using Umbraco.Core.Persistence;
@@ -293,5 +294,24 @@ namespace Umbraco.Tests.Persistence.NPocoTests
Debug.Print(sql.SQL);
}
[Test]
public void ForUpdate()
{
var sessionId = Guid.NewGuid();
var sql = Sql()
.SelectAll()
.From<UserLoginDto>()
.Where<UserLoginDto>(x => x.SessionId == sessionId);
sql.WriteToConsole();
Assert.AreEqual("SELECT * FROM [umbracoUserLogin] WHERE (([umbracoUserLogin].[sessionId] = @0))", sql.SQL.NoCrLf());
sql = sql.ForUpdate();
sql.WriteToConsole();
Assert.AreEqual("SELECT * FROM [umbracoUserLogin] WITH (UPDLOCK) WHERE (([umbracoUserLogin].[sessionId] = @0))", sql.SQL.NoCrLf());
}
}
}