Race Conditions in FSharp.Core #1505
Conversation
lastStamp was assigned to stamp outside the lock, which could have meant that stamp was assigned the same value if multiple threads were creating Var objects simultaneously. Moved it into a function to ensure access was limited to Interlocked function.
tables.Add is called from within a lock, so presumably this code has been written to attemp to be thread safe, thus tables.TryGetValue must also be protected by lock
accidently changed form int64 to int when fixing locking issue
| @@ -404,8 +404,10 @@ module internal Impl = | |||
| | _ -> invalidArg "tys" (SR.GetString(SR.invalidTupleTypes)) | |||
|
|
|||
| let tables = if isStruct then valueTupleTypes else refTupleTypes | |||
| match tables.TryGetValue(asm) with | |||
| match lock dictionaryLock (fun () -> tables.TryGetValue(asm)) with | |||
There was a problem hiding this comment.
I don't think this lock is necessary. We check tables outside of a lock to see if it contains asm If it is not in tables then we take a lock, recheck the dictionary and if it hasn't been added since we first checked, then we add it in. So as best I can tell there isn't really a race. And we have added an unnecessary lock per read.
Does that make sense?
Kevin
There was a problem hiding this comment.
Oh it makes sense what's is being attempted, of course, it is just not safe, because the Add can concurrently run on another thread and screw with internal dictionary data structures. The original Hashtable class supported that kind of concurrency, but that guarantee wasn't continued when the generic Dictionary was released. At least as far as my memory serves me. So I think it is an issue, but if you can point me to official documentation that says I'm wrong then I can be suitable enlightened.
There was a problem hiding this comment.
Now I'm confused, what makes sense. What I said, or what you said?
Just did a bit of an audit through FSharp.Core for threading primitives, and found a couple of spots where that had not been applied correctly (at least to my eyes).