search

jeffijoe / messageformat.net

ICU MessageFormat implementation for .NET.
MIT License
161 stars 23 forks source link

Race condition fix in MessageFormatter #10

Closed ramziyassine closed 7 years ago

ramziyassine commented 7 years ago

This resolves the exception listed https://github.com/jeffijoe/messageformat.net/issues/9

Overview

In addition to the notes listed in the above issue. I would like in the future to come back and propose some changes with MessageFormatter around standardizing the way we get instances of that class.

Today you can use the factory method (where the locks play a part) or you create instances via new but if client code shares those you will get the issue this PR is trying to solve.

I want to stress that this PR is geared towards preventing the exception I listed in the issue above only.

Code review consideration

Testing consideration

jeffijoe commented 7 years ago

Hmm, the tests fail on CI, it seems to be cause of a nuget issue.

image

ramziyassine commented 7 years ago

yup looking into that now, the project never had a package config trying to see if that will resolve it

jeffijoe commented 7 years ago

If you could, the readme needs editing - currently says:

It's portable. The library is a PCL, and has no dependencies - the only reference is the standard .NET in PCL's.

What do you suggest?

It's portable. The library is a PCL, and has just a single dependency (Portable.ConcurrentDictionary for thread safety) - other than that, the only reference is the standard .NET in PCL's.

You should be able to add new commits so we can merge them as one in this PR. :)

jeffijoe commented 7 years ago

Awesome! I'm gonna merge this and hopefully push to NuGet tomorrow! Thank you so much!

I would also like to add that I would take a cache miss over an exception any day! :shipit: Nice job @ramziyassine!

ramziyassine commented 7 years ago

Thank you sir 👍