-
Notifications
You must be signed in to change notification settings - Fork 491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CosmosDiagnostics: Fixes a race condition causing InvalidOperationException #2516
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only left 2 questions.
Microsoft.Azure.Cosmos/src/Tracing/TraceData/ClientSideRequestStatisticsTraceDatum.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Tracing/TraceWriter.TraceJsonWriter.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Tracing/TraceData/ClientSideRequestStatisticsTraceDatum.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The yiel under a lock is not thread-safe and needs to be changed.
…://github.com/Azure/azure-cosmos-dotnet-v3 into users/jawilley/iTrace/clientsideRaceCondition
Microsoft.Azure.Cosmos/src/Tracing/TraceData/ClientSideRequestStatisticsTraceDatum.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Tracing/TraceData/ClientSideRequestStatisticsTraceDatum.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Tracing/TraceData/ClientSideRequestStatisticsTraceDatum.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Jake - LGTM now - except for minor comments on simplifying the shallow copy implementation
…://github.com/Azure/azure-cosmos-dotnet-v3 into users/jawilley/iTrace/clientsideRaceCondition
…://github.com/Azure/azure-cosmos-dotnet-v3 into users/jawilley/iTrace/clientsideRaceCondition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now - Thanks Jake!
Pull Request Template
Description
Introduced in 3.17.0 PR #2097
This is a regression caused by making the ClientSideRequestStatisticsTraceDatum private fields to public. Previously these lists were protected by locks and when they were made public the locks were removed. This PR is adding the locks back and adding tests to ensure that if a background thread does update it while the list is being enumerated that it will not throw a InvalidOperationCancelledException.
Having a lock in the IEnumerable adds the possibility of deadlocks if the enumerator is not properly disposed of. To avoid this possibility a shallow copy of the list is lazily created. Once it is generated it will reuse the same shallow copy until the list is modified again.
Type of change
Please delete options that are not relevant.
Closing issues
To automatically close an issue: closes #IssueNumber