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

Error with EnumExtensions #5

Closed
xramcire opened this issue Feb 10, 2022 · 3 comments · Fixed by #6
Closed

Error with EnumExtensions #5

xramcire opened this issue Feb 10, 2022 · 3 comments · Fixed by #6
Labels
bug Something isn't working

Comments

@xramcire
Copy link
Contributor

Race conditions with EnumExtensions has a race condition causing the thread safe IDictionary.Add method to throw an error.

If 2 threads both access Enum.ToStringOrDefaultValue() for the same value, both will pass the "is not present check" and both will then attempt to add the value. This causes a "The key already existed in the dictionary." exception.

I can't do a commit to this repo so I have attached a potential fix / refactor of enum extensions.

fail: The key already existed in the dictionary.
System.ArgumentException: The key already existed in the dictionary.
at System.Collections.Concurrent.ConcurrentDictionary`2.System.Collections.Generic.IDictionary<TKey,TValue>.Add(TKey key, TValue value)
at BricklinkSharp.Client.Extensions.EnumExtensions.GetStringValueOrDefault(Enum e)
at BricklinkSharp.Client.BricklinkClient.GetItemAsync(ItemType type, String no, CancellationToken cancellationToken)

BricklinkSharp.zip

@gebirgslok
Copy link
Owner

Thanks for your contribution 👍.
Your solution is (besides the bug fix) also more efficient since we can get rid of the reflection.

I think the Obsolete Flag is not required since the extensions are internal anyway and you can remove the GetStringValueOrDefault methods. Also the StringValueAttribute is not needed anymore.

You can only commit to this repo via pull requests (PRs). Therefore you have to fork the repo into your personal space and commit into that repo. From there you can create PRs.

I can also merge it manually in the next days.

best

@gebirgslok gebirgslok added the bug Something isn't working label Feb 10, 2022
@xramcire
Copy link
Contributor Author

I'll get the PR sorted tomorrow.

@gebirgslok
Copy link
Owner

Thanks again for the PR.
I'll release a new version, but not quite sure whether this should be a 2.0.0 since the namespace for Enums changed (which needs to be adjusted) and I also found a typo in RatingTargeRole.
What do you think?

@gebirgslok gebirgslok linked a pull request Feb 12, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants