Dispose FSharpProjectPackage properly #2058
Conversation
| @@ -355,6 +355,14 @@ namespace rec Microsoft.VisualStudio.FSharp.ProjectSystem | |||
| pIdIco <- 400u | |||
| VSConstants.S_OK | |||
|
|
|||
| override this.Dispose(disposing) = | |||
There was a problem hiding this comment.
Will this always happen on a UI thread? should we used interlocked compex on componentID to ensure it's not double disposed, or is double disposing Okay?
There was a problem hiding this comment.
I guess I mean double revoking.
There was a problem hiding this comment.
We guard revoking component by componentId and mgr so I think double revoking should not happen.
There was a problem hiding this comment.
It doesn't look thread safe. But there may be other mitigations in place.
However, as it stands two threads could both invoke mgr..FRevokeComponent(componentID) simultaneously.
Now that might not be a problem ...
for example if mgr..FRevokeComponent(componentID) is resilient to double revoke.
or
Something higher up the stack is ensuring that two threads can't dispose simultaneous ... for example something enforces that only a single thread at a time is doing the dispose.
to make it threadsafe either grabbing a lock and double checking or using Interlocked.CompareExchange to set componentID back to zero would probably work fine.
There was a problem hiding this comment.
Good point. I added locking around revoking component bits.
Looking at the old code, the assumption is that the code should be invoked in a single thread https://github.com/dsyme/old-visualfsharp-clones/blob/8130b1429d3fd83a77a07df1bdc07751a6ee6577/vsintegration/src/vs/FsPkgs/FSharp.LanguageService/servicem.fs#L2069-L2078
There was a problem hiding this comment.
Looks good to me.
Thank you.
Kevin
| @@ -355,6 +355,14 @@ namespace rec Microsoft.VisualStudio.FSharp.ProjectSystem | |||
| pIdIco <- 400u | |||
| VSConstants.S_OK | |||
|
|
|||
| override this.Dispose(disposing) = | |||
There was a problem hiding this comment.
Looks good to me.
Thank you.
Kevin
This is supposed to be part of #2043.
Sorry for missing this earlier.