SLVS-2430 Trigger analysis on pressed key with debounce #6402
Conversation
gabriela-trutan-sonarsource
requested a review
from georgii-borovinskikh-sonarsource
gabriela-trutan-sonarsource
force-pushed
the
gt/real-time-analysis
branch
from
27202cd to
524fa48
Compare
| latestDebounceState = new Debounce(new CancellationTokenSource(), state); | ||
| } | ||
|
|
||
| var latestState = latestDebounceState; |
There was a problem hiding this comment.
This is very unexpected. 1) latestDebounceState should be accessed under lock only 2) it is cached before Task.Delay has completed. To simplify things, I'd suggest removing the T state from the contract make the task into Action without parameters. If we're assuming that the task with the latest state is going to win, we don't need to pass the latest state to it as it already has it
There was a problem hiding this comment.
- it is cached before Task.Delay has completed
It has to be cached before. So that the Task.Delay can be cancelled if newer text is being typed
There was a problem hiding this comment.
Removed the state from the contract
| try | ||
| { | ||
| await Task.Delay(TimeSpan.FromMilliseconds(debounceMilliseconds), latestState.CancellationTokenSource.Token); | ||
| if (!latestState.CancellationTokenSource.Token.IsCancellationRequested) |
There was a problem hiding this comment.
This check is redundant. Task delay is going to throw if the token is cancelled. This IF is going to be executed nanoseconds after the task delay completion, so there's no point in checking again
There was a problem hiding this comment.
All the instructions are in the order of nanoseconds. I would rather leave it
There was a problem hiding this comment.
Not really, the task delay is 1 second so there is definitely a high chance it's going to throw, while there's almost no chance the check below that would fail if it didn't
gabriela-trutan-sonarsource
requested a review
from georgii-borovinskikh-sonarsource
|
gabriela-trutan-sonarsource
merged commit ecc635c
into
feature/sqvs-roslyn-plugin
2 participants
SLVS-2430
This targets the PR in which
DocumentUpdatedevent was introduced