Fix potential race where we're expecting a moniker to be attached #65732
Conversation
| } | ||
| else if (symbol.ContainingAssembly.Equals(sourceCompilation.Assembly)) | ||
| { | ||
| kind = "export"; |
There was a problem hiding this comment.
what does 'export/import' mean here?
There was a problem hiding this comment.
Basically when we're indexing a project, we will stick a moniker for symbols we encounter. 'export' means it came from this project (and is visible elsewhere), 'import' means it came from another project. I'm not sure if the rest of the consumers really use this much, but that's the spec at least.
| // Attach the moniker vertex for this result set | ||
| _ = GetResultIdForSymbol(symbol, "moniker", () => monikerVertex); | ||
| } | ||
| _ = this.GetMoniker(symbol, _sourceCompilation); |
There was a problem hiding this comment.
i don't really understand this, but ok :)
There was a problem hiding this comment.
this work doesn't seem complex, is it actually bad to do under the lock?
There was a problem hiding this comment.
The lock earlier is a pretty high contention lock -- we have to hit it to look up a symbol -> id mapping every time we process any token, so it seems best to keep it out.
This change makes things a bit more robust anyways -- the other place assuming there was a moniker might also be wrong if for some reason we didn't have a moniker for that symbol so there's no real concern about the race anymore.
|
Ran this changeset locally against WebTools and it worked! /cc @jimmylewis |
The ResultSet provider already had an IdFactory in hand, so we can just use that.
jasonmalinowski
force-pushed
the
fix-lsif-issues
branch
from
440a356
to
09bd48b
Compare
When we're creating a ResultSet for a symbol, we always attach a moniker on it once it's created. We do that outside the main lock against the dictionary for all symbols, so that way the creation of the moniker isn't holding up operations for unrelated symbols. This was fine because nothing cared about the moniker existing, until we added additional support for inheritance, and the new spec now needs us to link to monikers in some cases. There I put in an incorrect assumption that the moniker might be there, which isn't the case if a different thread is still making it. Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1692879
jasonmalinowski
force-pushed
the
fix-lsif-issues
branch
from
09bd48b
to
82898be
Compare
ghost
added this to the 
When we're creating a ResultSet for a symbol, we always attach a moniker on it once it's created. We do that outside the main lock against the dictionary for all symbols, so that way the creation of the moniker isn't holding up operations for unrelated symbols. This was fine because nothing cared about the moniker existing, until we added additional support for inheritance, and the new spec now needs us to link to monikers in some cases. There I put in an incorrect assumption that the moniker might be there, which isn't the case if a different thread is still making it.
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1692879