Skip to content

Commit

Permalink
Add memory barries to writes at the end of a lock
Browse files Browse the repository at this point in the history
There could have been a race condition on systems with weak
memory ordering:

Another thread can read the result value before we exit the critical section.
Without a strong memory ordering, the pointer to the result value might get written before the value itself.
The other thread might therefore read zeroed-out memory.
  • Loading branch information
exyi committed Aug 18, 2024
1 parent 0097140 commit bf7ef61
Show file tree
Hide file tree
Showing 6 changed files with 22 additions and 7 deletions.
6 changes: 5 additions & 1 deletion src/Framework/Framework/Binding/DotvvmProperty.cs
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,11 @@ internal void AddUsedInCapability(DotvvmCapabilityProperty? p)
if (p is object)
lock(this)
{
UsedInCapabilities = UsedInCapabilities.Add(p);
if (UsedInCapabilities.Contains(p)) return;

var newArray = UsedInCapabilities.Add(p);
Thread.MemoryBarrier(); // make sure the array is complete before we let other threads use it lock-free
UsedInCapabilities = newArray;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ protected void AddNullResolvers()



string? toStringValue;
volatile string? toStringValue;
public override string ToString()
{
if (toStringValue is null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using DotVVM.Framework.Utils;
using System.Runtime.CompilerServices;
using System.Collections.Immutable;
using System.Threading;

namespace DotVVM.Framework.Compilation.ControlTree
{
Expand Down Expand Up @@ -81,8 +82,11 @@ internal void AddUsedInCapability(DotvvmCapabilityProperty? p)
if (p is object)
lock(this)
{
if (!UsedInCapabilities.Contains(p))
UsedInCapabilities = UsedInCapabilities.Add(p);
if (UsedInCapabilities.Contains(p)) return;

var newArray = UsedInCapabilities.Add(p);
Thread.MemoryBarrier();
UsedInCapabilities = newArray;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ public IEnumerable<MethodInfo> GetExtensionsForNamespaces(string[] @namespaces)
// it's most likely the same namespaces, so it won't help at all - only run into lock contention in System.Reflection
lock (methodsCache)
{
results = namespaces.Select(x => methodsCache.GetValueOrDefault(x)).ToArray();
var missingNamespaces = namespaces.Where(x => !methodsCache.ContainsKey(x)).ToArray();

var createdNamespaces = CreateExtensionsForNamespaces(missingNamespaces);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
using DotVVM.Framework.Compilation.ViewCompiler;
using DotVVM.Framework.Runtime;
using FastExpressionCompiler;
using System.Threading;

namespace DotVVM.Framework.Compilation.Styles
{
Expand Down Expand Up @@ -417,6 +418,7 @@ void InitializeChildren(IDotvvmRequestContext? context)
var services = context?.Services ?? this.services;

Children.Add((DotvvmControl)ResolvedControl.ToRuntimeControl(services));
Thread.MemoryBarrier(); // make sure write to Children is done before we let other threads read without lock
initialized = true;
}
}
Expand Down
10 changes: 8 additions & 2 deletions src/Framework/Framework/Runtime/Caching/SimpleLruDictionary.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ public class SimpleLruDictionary<TKey, TValue>
{
// concurrencyLevel: 1, we don't write in parallel anyway
// new generation
private ConcurrentDictionary<TKey, TValue> hot = new ConcurrentDictionary<TKey, TValue>(concurrencyLevel: 1, capacity: 1);
private volatile ConcurrentDictionary<TKey, TValue> hot = new ConcurrentDictionary<TKey, TValue>(concurrencyLevel: 1, capacity: 1);
// old generation
private ConcurrentDictionary<TKey, TValue> cold = new ConcurrentDictionary<TKey, TValue>(concurrencyLevel: 1, capacity: 1);
private volatile ConcurrentDictionary<TKey, TValue> cold = new ConcurrentDictionary<TKey, TValue>(concurrencyLevel: 1, capacity: 1);
// free to take for GC. however, if the GC does not want to collect, we can still use it
private readonly ConcurrentDictionary<TKey, WeakReference<TValue>> dead = new ConcurrentDictionary<TKey, WeakReference<TValue>>(concurrencyLevel: 1, capacity: 1);
private TimeSpan lastCleanupTime = TimeSpan.MinValue;
Expand Down Expand Up @@ -148,6 +148,12 @@ public bool Remove(TKey key, out TValue oldValue)
oldValue = deadValue;
r = true;
}
if (hot.TryRemove(key, out hotValue))
{
// hot again, it could have been added back in the meantime
oldValue = hotValue;
r = true;
}
return r;
}
}
Expand Down

0 comments on commit bf7ef61

Please sign in to comment.