Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Commit 7c50828

Browse files
authored
Fix race condition in DiagnosticSourceEventSource (#35269)
* Use RemoteInvoke in DiagnosticSourceEventListener tests Running these concurrently with other tests in the same process can result in spurious failures. * Fix race condition in DiagnosticSourceEventSource If multiple threads all try to write different objects at the same time, PropertySpec.Fetch can manifest a race condition that results in potentially trying to cast one call's object to another call's type. The fix is to allow for atomically storing the cached data, and using a local to ensure that a calling thread's view is consistent.
1 parent 5726c1e commit 7c50828

File tree

3 files changed

+584
-506
lines changed

3 files changed

+584
-506
lines changed

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticSourceEventSource.cs

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -800,13 +800,13 @@ public PropertySpec(string propertyName, PropertySpec next = null)
800800
public object Fetch(object obj)
801801
{
802802
Type objType = obj.GetType();
803-
if (objType != _expectedType)
803+
PropertyFetch fetch = _fetchForExpectedType;
804+
if (fetch == null || fetch.Type != objType)
804805
{
805-
var typeInfo = objType.GetTypeInfo();
806-
_fetchForExpectedType = PropertyFetch.FetcherForProperty(typeInfo.GetDeclaredProperty(_propertyName));
807-
_expectedType = objType;
806+
_fetchForExpectedType = fetch = PropertyFetch.FetcherForProperty(
807+
objType, objType.GetTypeInfo().GetDeclaredProperty(_propertyName));
808808
}
809-
return _fetchForExpectedType.Fetch(obj);
809+
return fetch.Fetch(obj);
810810
}
811811

812812
/// <summary>
@@ -820,21 +820,29 @@ public object Fetch(object obj)
820820
/// to efficiently fetch that property from a .NET object (See Fetch method).
821821
/// It hides some slightly complex generic code.
822822
/// </summary>
823-
class PropertyFetch
823+
private class PropertyFetch
824824
{
825+
protected PropertyFetch(Type type)
826+
{
827+
Debug.Assert(type != null);
828+
Type = type;
829+
}
830+
831+
internal Type Type { get; }
832+
825833
/// <summary>
826834
/// Create a property fetcher from a .NET Reflection PropertyInfo class that
827835
/// represents a property of a particular type.
828836
/// </summary>
829-
public static PropertyFetch FetcherForProperty(PropertyInfo propertyInfo)
837+
public static PropertyFetch FetcherForProperty(Type type, PropertyInfo propertyInfo)
830838
{
831839
if (propertyInfo == null)
832-
return new PropertyFetch(); // returns null on any fetch.
840+
return new PropertyFetch(type); // returns null on any fetch.
833841

834842
var typedPropertyFetcher = typeof(TypedFetchProperty<,>);
835843
var instantiatedTypedPropertyFetcher = typedPropertyFetcher.GetTypeInfo().MakeGenericType(
836844
propertyInfo.DeclaringType, propertyInfo.PropertyType);
837-
return (PropertyFetch)Activator.CreateInstance(instantiatedTypedPropertyFetcher, propertyInfo);
845+
return (PropertyFetch)Activator.CreateInstance(instantiatedTypedPropertyFetcher, type, propertyInfo);
838846
}
839847

840848
/// <summary>
@@ -844,9 +852,9 @@ public static PropertyFetch FetcherForProperty(PropertyInfo propertyInfo)
844852

845853
#region private
846854

847-
private class TypedFetchProperty<TObject, TProperty> : PropertyFetch
855+
private sealed class TypedFetchProperty<TObject, TProperty> : PropertyFetch
848856
{
849-
public TypedFetchProperty(PropertyInfo property)
857+
public TypedFetchProperty(Type type, PropertyInfo property) : base(type)
850858
{
851859
_propertyFetch = (Func<TObject, TProperty>)property.GetMethod.CreateDelegate(typeof(Func<TObject, TProperty>));
852860
}
@@ -860,8 +868,7 @@ public override object Fetch(object obj)
860868
}
861869

862870
private string _propertyName;
863-
private Type _expectedType;
864-
private PropertyFetch _fetchForExpectedType;
871+
private volatile PropertyFetch _fetchForExpectedType;
865872
#endregion
866873
}
867874

0 commit comments

Comments
 (0)