Fix a race condition in SymbolPool.Get

[jonathanvdc]

Hi!

I discovered a tiny race condition in SymbolPool.Get(string, out Symbol), which was hurting ecsc's ability to parse and macro-expand multiple EC# source code files in parallel (I actually had to switch back to sequential mode). This (small) pull request fixes said race condition, and adds a test case to verify. I've taken the liberty of including a reformatted version of the commit message below, to explain what went wrong and why my suggested fix tackles the issue in the way it does.

SymbolPool.Get used to check if _map contained the symbol's name, and then acquire a lock if that was not the case. Once inside the lock, the algorithm assumed that _map would still not contain the symbol. But it didn't account for the possibility that another thread could insert the symbol into _map in while the current thread was waiting for the lock. This sometimes resulted in an exception.

An extra check has been inserted in the lock statement to check that the assumption still holds before inserting a symbol in the map. It would have been simpler to move the initial check inside the lock statement, but that would penalize the hot path.

Does this look okay to you?

[jonathanvdc]

Oh, no. .NET 3.5 doesn't include System.Threading.Tasks.Parallel, but my test case relies on it. That sort of breaks the .NET 3.5 build. I'm not entirely sure how to deal with this. Are you okay with using preprocessor magic to ignore this test when compiling for .NET 3.5?

[qwertie]

Yes, please add #if !DotNet35 to avoid breaking the build. In Loyc I put regression tests in a test called Regression or BugFix plus a description of the bug. Please change to:

    [Test]
    public void BugFixOct2016_SymbolGetRaceCondition()
    {
        // Bug: SymbolPool.Get used to check the symbol table contained the symbol's 
        // name, and then acquire a lock if it didn't. Inside the lock it assumed that _map 
        // would still not contain the symbol and called Add(), which threw 
        // KeyAlreadyExistsException when the assumption was false.
        RunParallel(8, () =>
        {
            for (int i = 0; i < 1000; i++)
                GSymbol.Get(i.ToString());
        });
    }

[jonathanvdc]

I applied the changes you suggested, and AppVeyor tells me that the .NET 3.5 build is successful-ish now (a LES 3 lexer test case is failing, but I believe that issue predates this PR).