prevent exception from race condition in tryGetReflectedDefinition

[pezipink]

I have discovered a sort-of race condition within the FSharp.Core quotation library, that causes an "Item already exists..." exception to be thrown inside tryGetReflectedDefinition.

This PR simply prevents the exception from occurring. It does not address the problem itself, since I think it is worth further discussion. It is however causing me a problem in a production system, so I would appreciate the "band-aid" to at least prevent the exception.

The code in tryGetReflectedDefinition maintains a dictionary cache reflectedDefinitionTable and also a dictionary that appears to act as a hashset decodedTopResources.

The function syncs access to reflectedDefinitionTable by means of a lock, and where an item is missing it proceeds to load the binary quotations for the whole assembly, unpickle and then store the results within reflectedDefinitionTable. It also then remembers this assembly/resource has been processed by way of decodedTopResources.

The problem occurs when more than one thread hits this code path at the same time. All threads pass the initial lock since the requested key does not exist. This then causes all threads to (wastefully) unpickle the resources for the assembly. Finally, the first thread to reach the next locked section will populate the reflectedDefinitionTable and mark the assembly as processed via decodedTopResources. This locked section is cognizant that other threads might have already stored what they are looking for, however it does not account for the case where the key it is looking for does not exist (which will happen many times since it is invoked through the MethodWithReflectedDefinition pattern.) In this case, the thread will then wastefully re-add all the reflected definitions (which works since it uses the indexer) and try to re-add the decodedTopResources entries, causing an exception to be thrown since it uses the Add method.

There are various issues with the current design, but the cost of this function is tiny compared to deserialize and friends, so it might not be worth bothering to re-work it.

This change should have no impact, unless someone has a program relying on this exception in a threaded scenario ;) My current work around is to catch such errors and simply re-compile the quotations, which is far from ideal.

[Happypig375]

Bump @dsyme