Fix AsyncLocal<class> changed event to not be raised multiple times for one change...
authorKoundinya Veluri <kouvel@users.noreply.github.com>
Wed, 25 Apr 2018 22:21:52 +0000 (15:21 -0700)
committerGitHub <noreply@github.com>
Wed, 25 Apr 2018 22:21:52 +0000 (15:21 -0700)
Fix AsyncLocal<class> changed event to not be raised multiple times for one change in value

Functional fix for https://github.com/dotnet/coreclr/issues/17758

src/mscorlib/shared/System/Threading/AsyncLocal.cs
src/mscorlib/shared/System/Threading/ExecutionContext.cs

index 3531265..2bf8a04 100644 (file)
@@ -114,7 +114,7 @@ namespace System.Threading
     internal interface IAsyncLocalValueMap
     {
         bool TryGetValue(IAsyncLocal key, out object value);
-        IAsyncLocalValueMap Set(IAsyncLocal key, object value);
+        IAsyncLocalValueMap Set(IAsyncLocal key, object value, bool treatNullValueAsNonexistent);
     }
 
     //
@@ -124,14 +124,31 @@ namespace System.Threading
     {
         public static IAsyncLocalValueMap Empty { get; } = new EmptyAsyncLocalValueMap();
 
+        public static bool IsEmpty(IAsyncLocalValueMap asyncLocalValueMap)
+        {
+            Debug.Assert(asyncLocalValueMap != null);
+            Debug.Assert(asyncLocalValueMap == Empty || asyncLocalValueMap.GetType() != typeof(EmptyAsyncLocalValueMap));
+
+            return asyncLocalValueMap == Empty;
+        }
+
+        public static IAsyncLocalValueMap Create(IAsyncLocal key, object value, bool treatNullValueAsNonexistent)
+        {
+            // If the value isn't null or a null value may not be treated as nonexistent, then create a new one-element map
+            // to store the key/value pair.  Otherwise, use the empty map.
+            return value != null || !treatNullValueAsNonexistent ?
+                new OneElementAsyncLocalValueMap(key, value) :
+                Empty;
+        }
+
         // Instance without any key/value pairs.  Used as a singleton/
-        internal sealed class EmptyAsyncLocalValueMap : IAsyncLocalValueMap
+        private sealed class EmptyAsyncLocalValueMap : IAsyncLocalValueMap
         {
-            public IAsyncLocalValueMap Set(IAsyncLocal key, object value)
+            public IAsyncLocalValueMap Set(IAsyncLocal key, object value, bool treatNullValueAsNonexistent)
             {
-                // If the value isn't null, then create a new one-element map to store
-                // the key/value pair.  If it is null, then we're still empty.
-                return value != null ?
+                // If the value isn't null or a null value may not be treated as nonexistent, then create a new one-element map
+                // to store the key/value pair.  Otherwise, use the empty map.
+                return value != null || !treatNullValueAsNonexistent ?
                     new OneElementAsyncLocalValueMap(key, value) :
                     (IAsyncLocalValueMap)this;
             }
@@ -144,7 +161,7 @@ namespace System.Threading
         }
 
         // Instance with one key/value pair.
-        internal sealed class OneElementAsyncLocalValueMap : IAsyncLocalValueMap
+        private sealed class OneElementAsyncLocalValueMap : IAsyncLocalValueMap
         {
             private readonly IAsyncLocal _key1;
             private readonly object _value1;
@@ -154,21 +171,20 @@ namespace System.Threading
                 _key1 = key; _value1 = value;
             }
 
-            public IAsyncLocalValueMap Set(IAsyncLocal key, object value)
+            public IAsyncLocalValueMap Set(IAsyncLocal key, object value, bool treatNullValueAsNonexistent)
             {
-                if (value != null)
+                if (value != null || !treatNullValueAsNonexistent)
                 {
-                    // The value is non-null.  If the key matches one already contained in this map,
-                    // then create a new one-element map with the updated value, otherwise create
-                    // a two-element map with the additional key/value.
+                    // If the key matches one already contained in this map, then create a new one-element map with the updated
+                    // value, otherwise create a two-element map with the additional key/value.
                     return ReferenceEquals(key, _key1) ?
                         new OneElementAsyncLocalValueMap(key, value) :
                         (IAsyncLocalValueMap)new TwoElementAsyncLocalValueMap(_key1, _value1, key, value);
                 }
                 else
                 {
-                    // The value is null.  If the key exists in this map, remove it by downgrading to an empty map.
-                    // Otherwise, there's nothing to add or remove, so just return this map.
+                    // If the key exists in this map, remove it by downgrading to an empty map.  Otherwise, there's nothing to
+                    // add or remove, so just return this map.
                     return ReferenceEquals(key, _key1) ?
                         Empty :
                         (IAsyncLocalValueMap)this;
@@ -202,13 +218,12 @@ namespace System.Threading
                 _key2 = key2; _value2 = value2;
             }
 
-            public IAsyncLocalValueMap Set(IAsyncLocal key, object value)
+            public IAsyncLocalValueMap Set(IAsyncLocal key, object value, bool treatNullValueAsNonexistent)
             {
-                if (value != null)
+                if (value != null || !treatNullValueAsNonexistent)
                 {
-                    // The value is non-null.  If the key matches one already contained in this map,
-                    // then create a new two-element map with the updated value, otherwise create
-                    // a three-element map with the additional key/value.
+                    // If the key matches one already contained in this map, then create a new two-element map with the updated
+                    // value, otherwise create a three-element map with the additional key/value.
                     return
                         ReferenceEquals(key, _key1) ? new TwoElementAsyncLocalValueMap(key, value, _key2, _value2) :
                         ReferenceEquals(key, _key2) ? new TwoElementAsyncLocalValueMap(_key1, _value1, key, value) :
@@ -216,8 +231,8 @@ namespace System.Threading
                 }
                 else
                 {
-                    // The value is null.  If the key exists in this map, remove it by downgrading to a one-element map
-                    // without the key.  Otherwise, there's nothing to add or remove, so just return this map.
+                    // If the key exists in this map, remove it by downgrading to a one-element map without the key.  Otherwise,
+                    // there's nothing to add or remove, so just return this map.
                     return
                         ReferenceEquals(key, _key1) ? new OneElementAsyncLocalValueMap(_key2, _value2) :
                         ReferenceEquals(key, _key2) ? new OneElementAsyncLocalValueMap(_key1, _value1) :
@@ -258,12 +273,12 @@ namespace System.Threading
                 _key3 = key3; _value3 = value3;
             }
 
-            public IAsyncLocalValueMap Set(IAsyncLocal key, object value)
+            public IAsyncLocalValueMap Set(IAsyncLocal key, object value, bool treatNullValueAsNonexistent)
             {
-                if (value != null)
+                if (value != null || !treatNullValueAsNonexistent)
                 {
-                    // The value is non-null.  If the key matches one already contained in this map,
-                    // then create a new three-element map with the updated value.
+                    // If the key matches one already contained in this map, then create a new three-element map with the
+                    // updated value.
                     if (ReferenceEquals(key, _key1)) return new ThreeElementAsyncLocalValueMap(key, value, _key2, _value2, _key3, _value3);
                     if (ReferenceEquals(key, _key2)) return new ThreeElementAsyncLocalValueMap(_key1, _value1, key, value, _key3, _value3);
                     if (ReferenceEquals(key, _key3)) return new ThreeElementAsyncLocalValueMap(_key1, _value1, _key2, _value2, key, value);
@@ -279,8 +294,8 @@ namespace System.Threading
                 }
                 else
                 {
-                    // The value is null.  If the key exists in this map, remove it by downgrading to a two-element map
-                    // without the key.  Otherwise, there's nothing to add or remove, so just return this map.
+                    // If the key exists in this map, remove it by downgrading to a two-element map without the key.  Otherwise,
+                    // there's nothing to add or remove, so just return this map.
                     return
                         ReferenceEquals(key, _key1) ? new TwoElementAsyncLocalValueMap(_key2, _value2, _key3, _value3) :
                         ReferenceEquals(key, _key2) ? new TwoElementAsyncLocalValueMap(_key1, _value1, _key3, _value3) :
@@ -332,17 +347,18 @@ namespace System.Threading
                 _keyValues[index] = new KeyValuePair<IAsyncLocal, object>(key, value);
             }
 
-            public IAsyncLocalValueMap Set(IAsyncLocal key, object value)
+            public IAsyncLocalValueMap Set(IAsyncLocal key, object value, bool treatNullValueAsNonexistent)
             {
                 // Find the key in this map.
                 for (int i = 0; i < _keyValues.Length; i++)
                 {
                     if (ReferenceEquals(key, _keyValues[i].Key))
                     {
-                        // The key is in the map.  If the value isn't null, then create a new map of the same
-                        // size that has all of the same pairs, with this new key/value pair overwriting the old.
-                        if (value != null)
+                        // The key is in the map.
+                        if (value != null || !treatNullValueAsNonexistent)
                         {
+                            // Create a new map of the same size that has all of the same pairs, with this new key/value pair
+                            // overwriting the old.
                             var multi = new MultiElementAsyncLocalValueMap(_keyValues.Length);
                             Array.Copy(_keyValues, 0, multi._keyValues, 0, _keyValues.Length);
                             multi._keyValues[i] = new KeyValuePair<IAsyncLocal, object>(key, value);
@@ -350,8 +366,8 @@ namespace System.Threading
                         }
                         else if (_keyValues.Length == 4)
                         {
-                            // The value is null, and we only have four elements, one of which we're removing,
-                            // so downgrade to a three-element map, without the matching element.
+                            // We only have four elements, one of which we're removing, so downgrade to a three-element map,
+                            // without the matching element.
                             return
                                 i == 0 ? new ThreeElementAsyncLocalValueMap(_keyValues[1].Key, _keyValues[1].Value, _keyValues[2].Key, _keyValues[2].Value, _keyValues[3].Key, _keyValues[3].Value) :
                                 i == 1 ? new ThreeElementAsyncLocalValueMap(_keyValues[0].Key, _keyValues[0].Value, _keyValues[2].Key, _keyValues[2].Value, _keyValues[3].Key, _keyValues[3].Value) :
@@ -360,8 +376,8 @@ namespace System.Threading
                         }
                         else
                         {
-                            // The value is null, and we have enough elements remaining to warrant a multi map.
-                            // Create a new one and copy all of the elements from this one, except the one to be removed.
+                            // We have enough elements remaining to warrant a multi map.  Create a new one and copy all of the
+                            // elements from this one, except the one to be removed.
                             var multi = new MultiElementAsyncLocalValueMap(_keyValues.Length - 1);
                             if (i != 0) Array.Copy(_keyValues, 0, multi._keyValues, 0, i);
                             if (i != _keyValues.Length - 1) Array.Copy(_keyValues, i + 1, multi._keyValues, i, _keyValues.Length - i - 1);
@@ -372,9 +388,9 @@ namespace System.Threading
 
                 // The key does not already exist in this map.
 
-                // If the value is null, then we can simply return this same map, as there's nothing to add or remove.
-                if (value == null)
+                if (value == null && treatNullValueAsNonexistent)
                 {
+                    // We can simply return this same map, as there's nothing to add or remove.
                     return this;
                 }
 
@@ -418,14 +434,14 @@ namespace System.Threading
         {
             public ManyElementAsyncLocalValueMap(int capacity) : base(capacity) { }
 
-            public IAsyncLocalValueMap Set(IAsyncLocal key, object value)
+            public IAsyncLocalValueMap Set(IAsyncLocal key, object value, bool treatNullValueAsNonexistent)
             {
                 int count = Count;
                 bool containsKey = ContainsKey(key);
 
                 // If the value being set exists, create a new many map, copy all of the elements from this one,
                 // and then store the new key/value pair into it.  This is the most common case.
-                if (value != null)
+                if (value != null || !treatNullValueAsNonexistent)
                 {
                     var map = new ManyElementAsyncLocalValueMap(count + (containsKey ? 0 : 1));
                     foreach (KeyValuePair<IAsyncLocal, object> pair in this)
@@ -436,15 +452,14 @@ namespace System.Threading
                     return map;
                 }
 
-                // Otherwise, the value is null, which means null is being stored into an AsyncLocal.Value.
-                // Since there's no observable difference at the API level between storing null and the key
-                // not existing at all, we can downgrade to a smaller map rather than storing null.
+                // Otherwise, the value is null and a null value may be treated as nonexistent. We can downgrade to a smaller
+                // map rather than storing null.
 
                 // If the key is contained in this map, we're going to create a new map that's one pair smaller.
                 if (containsKey)
                 {
                     // If the new count would be within range of a multi map instead of a many map,
-                    // downgrade to the many map, which uses less memory and is faster to access.
+                    // downgrade to the multi map, which uses less memory and is faster to access.
                     // Otherwise, just create a new many map that's missing this key.
                     if (count == MultiElementAsyncLocalValueMap.MaxMultiElements + 1)
                     {
@@ -475,8 +490,8 @@ namespace System.Threading
                     }
                 }
 
-                // We were storing null, but the key wasn't in the map, so there's nothing to change.
-                // Just return this instance.
+                // We were storing null and a null value may be treated as nonexistent, but the key wasn't in the map, so
+                // there's nothing to change.  Just return this instance.
                 return this;
             }
         }
index a840aa7..975ed02 100644 (file)
@@ -64,8 +64,7 @@ namespace System.Threading
         {
             Debug.Assert(isFlowSuppressed != m_isFlowSuppressed);
 
-            if (m_localValues == null ||
-                m_localValues.GetType() == typeof(AsyncLocalValueMap.EmptyAsyncLocalValueMap))
+            if (m_localValues == null || AsyncLocalValueMap.IsEmpty(m_localValues))
             {
                 return isFlowSuppressed ?
                     DefaultFlowSuppressed :
@@ -322,19 +321,26 @@ namespace System.Threading
                 return;
             }
 
+            // Regarding 'treatNullValueAsNonexistent: !needChangeNotifications' below:
+            // - When change notifications are not necessary for this IAsyncLocal, there is no observable difference between
+            //   storing a null value and removing the IAsyncLocal from 'm_localValues'
+            // - When change notifications are necessary for this IAsyncLocal, the IAsyncLocal's absence in 'm_localValues'
+            //   indicates that this is the first value change for the IAsyncLocal and it needs to be registered for change
+            //   notifications. So in this case, a null value must be stored in 'm_localValues' to indicate that the IAsyncLocal
+            //   is already registered for change notifications.
             IAsyncLocal[] newChangeNotifications = null;
             IAsyncLocalValueMap newValues;
             bool isFlowSuppressed = false;
             if (current != null)
             {
                 isFlowSuppressed = current.m_isFlowSuppressed;
-                newValues = current.m_localValues.Set(local, newValue);
+                newValues = current.m_localValues.Set(local, newValue, treatNullValueAsNonexistent: !needChangeNotifications);
                 newChangeNotifications = current.m_localChangeNotifications;
             }
             else
             {
                 // First AsyncLocal
-                newValues = new AsyncLocalValueMap.OneElementAsyncLocalValueMap(local, newValue);
+                newValues = AsyncLocalValueMap.Create(local, newValue, treatNullValueAsNonexistent: !needChangeNotifications);
             }
 
             //
@@ -360,7 +366,7 @@ namespace System.Threading
             }
 
             Thread.CurrentThread.ExecutionContext = 
-                (!isFlowSuppressed && newValues.GetType() == typeof(AsyncLocalValueMap.EmptyAsyncLocalValueMap)) ?
+                (!isFlowSuppressed && AsyncLocalValueMap.IsEmpty(newValues)) ?
                 null : // No values, return to Default context
                 new ExecutionContext(newValues, newChangeNotifications, isFlowSuppressed);