search

sillsdev / icu-dotnet

C# wrapper for ICU4C
MIT License
62 stars 33 forks source link

Fix race conditions in native methods #164

Closed cbersch closed 2 years ago

cbersch commented 2 years ago

The containers for caching native method delegates must not be initialized at runtime without locking. This allows for race conditions and can lead to NullRefException when calling two methods simultaneously which require the same methods container.

Alternatively to the current solution, I could have locked when creating the methods container. But I don't see any reason for doing this. So I decided to go with always creating the methods containers.

I added one exemplary test which fails without the fix.

cbersch commented 2 years ago

It was actually the Wrapper.Cleanup() at the beginning of the new test which broke the other tests. A simple test

[Test, NonParallelizable]
public void CleanupTest()
{
    Wrapper.Cleanup();
}

in the BreakIteratorTests.cs breaks 30 other tests when running on Linux. Same behavior on the master and my branch.

I thought, that I could safely invoke that, since a cleanup is also done in a few other tests. I reverted that new test since I couldn't figure out, how to write the test I wanted to.

ermshiperete commented 2 years ago

Applied in commit bb1a46effb369ebf540a20d15cbbdf12c6a2757d.

Thanks!