Skip to content

Commit c145985

Browse files
Diagnostics: Fixes possible Thread contention in Traces (#5455)
# Pull Request Template ## Description This PR fixes a regression in tracing introduced by #5422, which can lead to thread contention. The bug is that the locking expects Trace-names to be unique for different logical operations - but many of the strings passed into new root traces (logical operations) are not new string instances (with possibly same value) but constants - so same singelton string instances. In tehse cases the new code will provide unnecessary lock contention. The locking scope should be just the scope of the ITrace instance - so, this PR introduces a new instance-level lock object (something I tried to avoid initially to avoid allocation of a new object-instance per trace). The alternative (Id - is a ValueType) and (children is a List that is exposed via properties - locking it would not be guaranteed to be dead-lock free - woudl be unliekly but not impossble if caller locks on the returned value from the Childred property). ## Type of change Please delete options that are not relevant. - [x] Bug fix (non-breaking change which fixes an issue) - [] New feature (non-breaking change which adds functionality) - [] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [] This change requires a documentation update ## Closing issues To automatically close an issue: closes #IssueNumber
1 parent 75175de commit c145985

1 file changed

Lines changed: 7 additions & 5 deletions

File tree

  • Microsoft.Azure.Cosmos/src/Tracing

Microsoft.Azure.Cosmos/src/Tracing/Trace.cs

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ namespace Microsoft.Azure.Cosmos.Tracing
1212
internal sealed class Trace : ITrace
1313
{
1414
private static readonly IReadOnlyDictionary<string, object> EmptyDictionary = new Dictionary<string, object>();
15+
private readonly object lockObject;
1516
private volatile List<ITrace> children;
1617
private volatile Dictionary<string, object> data;
1718
private ValueStopwatch stopwatch;
@@ -25,6 +26,7 @@ private Trace(
2526
TraceSummary summary)
2627
{
2728
this.Name = name ?? throw new ArgumentNullException(nameof(name));
29+
this.lockObject = new object();
2830
this.Id = Guid.NewGuid();
2931
this.StartTime = DateTime.UtcNow;
3032
this.stopwatch = ValueStopwatch.StartNew();
@@ -122,7 +124,7 @@ public ITrace StartChild(
122124

123125
public void AddChild(ITrace child)
124126
{
125-
lock (this.Name)
127+
lock (this.lockObject)
126128
{
127129
if (!this.isBeingWalked)
128130
{
@@ -172,7 +174,7 @@ public void AddDatum(string key, TraceDatum traceDatum)
172174

173175
public void AddDatum(string key, object value)
174176
{
175-
lock (this.Name)
177+
lock (this.lockObject)
176178
{
177179
this.data ??= new Dictionary<string, object>();
178180

@@ -192,7 +194,7 @@ public void AddDatum(string key, object value)
192194

193195
public void AddOrUpdateDatum(string key, object value)
194196
{
195-
lock (this.Name)
197+
lock (this.lockObject)
196198
{
197199
this.data ??= new Dictionary<string, object>();
198200

@@ -217,7 +219,7 @@ internal void SetWalkingStateRecursively()
217219
return; // Already set, return early
218220
}
219221

220-
lock (this.Name)
222+
lock (this.lockObject)
221223
{
222224
if (this.isBeingWalked)
223225
{
@@ -239,7 +241,7 @@ internal void SetWalkingStateRecursively()
239241

240242
bool ITrace.TryGetDatum(string key, out object datum)
241243
{
242-
lock (this.Name)
244+
lock (this.lockObject)
243245
{
244246
if (this.data == null)
245247
{

0 commit comments

Comments
 (0)