Make incremental builder counter atomic #724
Conversation
src/fsharp/vs/IncrementalBuild.fs
Outdated
| referenceCount <- referenceCount - 1 | ||
| if referenceCount = 0 then | ||
| System.Threading.Interlocked.Decrement(referenceCount) |> ignore | ||
| if !referenceCount = 0 then |
There was a problem hiding this comment.
This may race if someone else modifies it between these calls. I think the return value of Interlocked.Decrement should be used instead
src/fsharp/vs/IncrementalBuild.fs
Outdated
| @@ -1901,5 +1901,5 @@ type IncrementalBuilder(ctokCtor: CompilationThreadToken, frameworkTcImportsCach | |||
| | Some builder -> builder.IncrementUsageCount() | |||
There was a problem hiding this comment.
What happens if it is decremented to 0 between getting it, and incrementing the count?
Then it may have been disposed already, but this would set the counter to 1 again.
I think you would need a central routine, which increments the counter first, checks if it was > 0 before, then returns Some (or None if it was at zero).
src/fsharp/vs/IncrementalBuild.fs
Outdated
| @@ -1367,7 +1367,7 @@ type IncrementalBuilder(ctokCtor: CompilationThreadToken, frameworkTcImportsCach | |||
| let assertNotDisposed() = | |||
| if disposed then | |||
| System.Diagnostics.Debug.Assert(false, "IncrementalBuild object has already been disposed!") | |||
| let mutable referenceCount = 0 | |||
| let referenceCount = ref 0 | |||
There was a problem hiding this comment.
I think the count should start at 1. Decrementing it to 0 triggers a dispose, and incrementing it from 0 to 1 is illegal, because then the object has already been disposed in another thread..
There was a problem hiding this comment.
I think no one has access before the first increment.
There was a problem hiding this comment.
(Nit: This doesn't need to be a ref - you can take the address of the mutable field)
There was a problem hiding this comment.
Yes, but if you start at 1, then you can later make it illegal to increment from 0 to 1. (Because at 0, the object has already been disposed). This is just a preference.
There was a problem hiding this comment.
For this PR we're just fixing the race condition, so let's stick with 0 :)
|
Pushed update:
|
|
I think it can still race at places like this: https://github.com/fsharp/FSharp.Compiler.Service/blob/b95f6fa386405ffba0cae9e3d6d60302dcaf2a2c/src/fsharp/vs/service.fs#L2569 It is first taken from the cache, then the count is incremented. But someone else could have disposed it between these two lines. |
|
@0x53A I see. Perhaps getOrCreateBuilder and the creation process should return the decrementer |
|
I think I'd like to get this merged as soon as possible as it's critical fix for Ionide, and, as far as I can tell, it's "good enough". We can investigate more fixes later, if it's still problem. |
|
@Krzysztof-Cieslak yes I will merge once green and fix up when releasing |
|
@0x53A thanks for the careful review |
|
Travis partial failures ... seems unrelated ;) |
|
@Krzysztof-Cieslak If there are unrelated failures, you can "kick" the build agents by closing and reopening a PR. This will cause all the build agents to rebuild, and you can get rid of a spurious build failure that way. |
* remove redundant explicit copy * fix build
Fix #716