Utilize IVsAsyncFileChangeEx2 #70936
Conversation
This interface allows a more efficient mechanism by allowing for batch advising to file change events. Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1870752
ghost
added
Area-IDE
untriaged
|
@jasonmalinowski -- are you the right person to take a look? |
|
@ToddGrun I am! sorry for missing the ping here. |
| case Kind.WatchFiles: | ||
| var cookies = await service.AdviseFileChangesAsync(_files, _fileChangeFlags, _sink, cancellationToken).ConfigureAwait(false); | ||
|
|
||
| Contract.ThrowIfTrue(cookies.Length != _tokens.Length); |
There was a problem hiding this comment.
If this throws, it would mean the cookies that were returned will then be leaked. This really shouldn't happen though, but do we want to have some sort of cleanup in that case? Or at least should we really crash the process since something went very wrong?
There was a problem hiding this comment.
I think this is along the lines of something that should never happen (since we know from our code that _paths and _tokens are of the same length). If they give us back a different number of cookies that paths we sent, then they are being super bad and probably are causing issues in all callers. I think I'd like to just keep this as is unless you feel strongly.
There was a problem hiding this comment.
Signing off as I think this looks fine, but there might be a nice follow-up here. Now we have four states, for Watch/Unwatch and single file versus files. But if we use something like OneOrMany to hold the single/many we can reduce down to two states and probably simplify the code a bit further. I'll leave it to you to decide if that's worth doing now (or not.)
| switch (op._kind) | ||
| { |
There was a problem hiding this comment.
What worries me most with this is whether we'd accidentally forget to include one of the fields in one of the branches, meaning a combining might drop something. Is there any reason we can't do something like just checking _token for whether there is one, or just adding _tokens (which will be empty and probably a no-op)?
There was a problem hiding this comment.
Wasn't clear on the ask here, are you still wanting this change?
There was a problem hiding this comment.
So my worry would be if one of the branches here forgot to add something to a builder -- it'd mean that an operation would work fine by itself, but once we merged it something might get lost.
My thought though: do we need the switch? Because if you just took the code in the WatchFiles branch (which also covers Unwatch files) and the UnwatchDirectories, does it just work for all scenarios?
There was a problem hiding this comment.
I don't think I'd want to merge these codepaths. One concern is that some of these fields could be null (eg, op.__cookies can be null). It's not a lot of code, and I think it makes it clearer if they are separated.
|
@jasonmalinowski -- would love to get your eyes on this again since I changed it a bit from the last time you looked. Thanks! |
| switch (op._kind) | ||
| { |
There was a problem hiding this comment.
So my worry would be if one of the branches here forgot to add something to a builder -- it'd mean that an operation would work fine by itself, but once we merged it something might get lost.
My thought though: do we need the switch? Because if you just took the code in the WatchFiles branch (which also covers Unwatch files) and the UnwatchDirectories, does it just work for all scenarios?
1) simplify CanCombineWith 2) Use ArrayBuilder.Add instead of AddRange 3) Verify collection count before access
3 participants
This interface allows a more efficient mechanism by allowing for batch advising to file change events.
Fixes AB#1870752