Skip to content
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

Fix potential ArgumentException with ConditionalWeakTable #8444

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

gpetrou
Copy link
Contributor

@gpetrou gpetrou commented Jan 8, 2025

In some rare occasions, we are observing System.ArgumentException: An item with the same key has already been added. at System.Runtime.CompilerServices.ConditionalWeakTable``2.Add(TKey key, TValue value) when adding entries to SettingsTable.
I added a lock similar to https://github.com/elastic/apm-agent-dotnet/blob/c4c0649dc18ad15fd89f9a878db8c1b80a103dab/src/Elastic.Apm/Logging/ScopedLogger.cs#L15 to address this issue.

@gpetrou gpetrou force-pushed the ConditionalWeakTable branch from f13f180 to 8f8859c Compare January 8, 2025 15:47
@flobernd
Copy link
Member

flobernd commented Jan 8, 2025

Hi @gpetrou , thanks a lot for the PR. LGTM! This will be included in the next release.

@gpetrou gpetrou force-pushed the ConditionalWeakTable branch from 3df09b1 to f88ffcf Compare January 9, 2025 06:36
@gpetrou
Copy link
Contributor Author

gpetrou commented Jan 9, 2025

@flobernd I ended up using a newer API for .NET 8.

@flobernd
Copy link
Member

flobernd commented Jan 9, 2025

Thank you! That's even better.

@gpetrou
Copy link
Contributor Author

gpetrou commented Jan 13, 2025

@flobernd are you going to create a new release with this change this week?

@flobernd
Copy link
Member

@gpetrou Yes, there will be a release this week. Not sure about the exact day yet.

@flobernd flobernd merged commit d06249c into elastic:main Jan 13, 2025
17 of 18 checks passed
flobernd added a commit that referenced this pull request Jan 13, 2025
@gpetrou gpetrou deleted the ConditionalWeakTable branch January 13, 2025 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants