Fix incorrect task chaining in InlineRenameSession by sharwell · Pull Request #34254 · dotnet/roslyn

sharwell · 2019-03-19T17:56:39Z

sharwell · 2019-03-19T17:58:07Z

📝 QueueApplyReplacements is chained on _allRenameLocationsTask, but depends on data produced by RaiseSessionSpansUpdated. If the tasks are reordered (e.g. if a Task.Delay is inserted above before RaiseSessionSpansUpdated), the data will be processed incorrectly and lead to a failure.

i'm not understanding. if QueueApplyReplacements is chained on _allRenameLocationsTask, then shouldn't it only run when _allRenameLocationsTask completes? And wouldn't htat only complete once that lambda completes? Which only finishes after RaiseSessionSpansUpdated runs and finishes? Even if something caused a delay before/after RaiseSessionSpansUpdated what would change?

Can you clarify your explanation?

And wouldn't htat only complete once that lambda completes?

You are confusing _allRenameLocationsTask (the field) with allRenameLocationsTask (the local variable). 😉

oh boy. i'll reread, but that certainly is a confusing way to name things.

jasonmalinowski

There is a deadlock introduced by this approach.

Pull request changed since original problem.

QueueApplyReplacements is chained on _allRenameLocationsTask, but depends on data produced by RaiseSessionSpansUpdated. If the tasks are reordered (e.g. if a Task.Delay is inserted above before RaiseSessionSpansUpdated), the data will be processed incorrectly and lead to a failure. Fixes dotnet#34221

RikkiGibson · 2019-06-06T20:19:02Z

@dotnet/roslyn-ide this could help with test reliability. Could we get this reviewed?

JoeRobich

Looks good to me, but I can't speak for the JTF usage.

Closes dotnet#38247 Closes dotnet#38555

jasonmalinowski

So I agree this does fix the task chaining problem, but I regret it doesn't actually simplify all the crazy state in the first place. If nothing else, can we add a comment or two at the points where we're updating the chain to describe why it's important to have the ordering? I'm happy to sign off if this is done.

I'm also worried about the perf impact of this: previously, it was OK for you to start an inline rename, type something, and hit enter really fast, the task chain is directly computing the original locations, then the conflict resolution, and then will go and apply that. This is now sticking in the middle of that the UI update for something the user won't ever actually see. I don't know how often that happens of course.

sharwell · 2019-09-25T14:45:04Z

If nothing else, can we add a comment or two at the points where we're updating the chain to describe why it's important to have the ordering?

@jasonmalinowski This is now done. I also corrected an incorrect cancellation token use, with documentation to clarify why CancellationToken.None is correct even though another cancellation is available in the current scope.

RikkiGibson · 2019-10-03T19:53:15Z

@dotnet/roslyn-ide for review.

jasonmalinowski

Not understanding why we're trying to maintain the LazyCancellation behavior -- it seems like it's making the conversion more complicated than necessary...

jasonmalinowski · 2019-10-07T23:55:21Z

+                UpdateReferenceLocationsTask(ThreadingContext.JoinableTaskFactory.RunAsync(async () =>
+                {
+                    // Join prior work before proceeding, since it performs a required state update.
+                    // https://github.com/dotnet/roslyn/pull/34254#discussion_r267024593


Not sure if it's worth inlining the explanation rather than relying on GitHub not breaking a URL like that....

jasonmalinowski · 2019-10-08T00:01:04Z

+                    //
+                    // The cancellation token is passed to the prior work when it starts, not when it's joined. This is
+                    // the equivalent of TaskContinuationOptions.LazyCancellation.
+                    await _allRenameLocationsTask.JoinAsync(CancellationToken.None).ConfigureAwait(false);


Why wouldn't this just pass the cancellationToken? I think you're trying to preserve behavior, but do we need to?

(I never liked that LazyCancellation was forced on everybody using SafeContinueWith who didn't perhaps need it because it means we would be potentially cancelling things a bit later.)

A behavior change here requires proof that the newly-allowed concurrency would not result in state corruption.

FindRenameLocationsAsync already can be running multiple times in parallel if for example options were changed in rapid succession multiple times. Either it's already safe for concurrency (and it's not a concern) or it's not, and then this is still bugged.

I was not able to prove that this one would behave correctly with lazy cancellation. With that said, this cancellation token only gets cancelled when the rename session is cancelled or committed, so it won't be impacting typing performance. If/when it is cancelled, the remaining tasks will quickly cancel themselves after the currently executing task finishes (by cancelling).

With that said, this cancellation token only gets cancelled when the rename session is cancelled or committed, so it won't be impacting typing performance.

Ah, maybe this is why I was confused earlier: this code has more problems than what you're trying to fix. 😄 If the options are changed, we should have entirely cancelled off the old work because there's no reason for that to continue running. What this should be doing (loosely) is cancelling _cancellationTokenSource and creating a new one before it calls UpdateReferenceLocationsTask. It appears this predates the open sourcing of this so I don't know why this ended up this way.

Want to file a bug? It's not that hard to fix but requires some care and I don't blame you for not wanting to fix it in this PR. (Although you're welcome to!)

Filed #40891

sharwell · 2019-12-31T21:45:12Z

@jasonmalinowski to re-review the final updates

jasonmalinowski

Looks good but I think I understand some of our confusion earlier: there's an (existing) perf issue where an option change should be cancelling off the old reference location search but isn't. Please file a bug for that while closing this, as that's a very not good bug.

jasonmalinowski · 2020-01-10T00:16:52Z

+                await ThreadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(alwaysYield: true, _cancellationTokenSource.Token);
+                _cancellationTokenSource.Token.ThrowIfCancellationRequested();
+
+                RaiseSessionSpansUpdated(inlineRenameLocations.Locations.ToImmutableArray());


Can we get a comment here saying it's unfortunate that _allRenameLocationsTask now requires the UI thread before it can let continuations run? Don't get me wrong: this is correctly expressing how it has to work right now, but it's still not how it was supposed to work.

Added a comment and linked it to #40890

jasonmalinowski · 2020-01-10T00:30:29Z

+                    //
+                    // The cancellation token is passed to the prior work when it starts, not when it's joined. This is
+                    // the equivalent of TaskContinuationOptions.LazyCancellation.
+                    await _allRenameLocationsTask.JoinAsync(CancellationToken.None).ConfigureAwait(false);


With that said, this cancellation token only gets cancelled when the rename session is cancelled or committed, so it won't be impacting typing performance.

Ah, maybe this is why I was confused earlier: this code has more problems than what you're trying to fix. 😄 If the options are changed, we should have entirely cancelled off the old work because there's no reason for that to continue running. What this should be doing (loosely) is cancelling _cancellationTokenSource and creating a new one before it calls UpdateReferenceLocationsTask. It appears this predates the open sourcing of this so I don't know why this ended up this way.

Want to file a bug? It's not that hard to fix but requires some care and I don't blame you for not wanting to fix it in this PR. (Although you're welcome to!)

sharwell requested a review from a team as a code owner March 19, 2019 17:56

sharwell commented Mar 19, 2019

View reviewed changes

sharwell force-pushed the task-chaining branch from db15d0d to cc32a8e Compare March 19, 2019 17:59

jasonmalinowski previously requested changes Mar 19, 2019

View reviewed changes

Comment thread src/EditorFeatures/Core.Wpf/InlineRename/InlineRenameSession.cs Outdated

sharwell force-pushed the task-chaining branch from cc32a8e to a9200bc Compare March 19, 2019 18:52

jinujoseph added the Area-IDE label Mar 20, 2019

sharwell added 2 commits May 15, 2019 16:44

Fix threading rule violations for UI-dependent tasks

d71163e

sharwell force-pushed the task-chaining branch from 5b613d3 to d71163e Compare May 15, 2019 21:44

JoeRobich approved these changes Jun 11, 2019

View reviewed changes

Comment thread src/EditorFeatures/Core.Wpf/InlineRename/InlineRenameSession.cs Outdated

Comment thread src/EditorFeatures/Core.Wpf/InlineRename/InlineRenameSession.cs Outdated

jinujoseph mentioned this pull request Aug 9, 2019

Flaky Test CSharp_FixupSpanDuringResolvableConflict_ReferenceConflict #37834

Closed

sharwell added 2 commits September 10, 2019 12:26

Merge remote-tracking branch 'dotnet/master' into task-chaining

46bc0bc

Enable fixed tests

a38d48c

Closes dotnet#38247 Closes dotnet#38555

jasonmalinowski reviewed Sep 11, 2019

View reviewed changes

Comment thread src/EditorFeatures/Core.Wpf/InlineRename/InlineRenameSession.cs

Comment thread src/EditorFeatures/Core.Wpf/InlineRename/InlineRenameSession.cs Outdated

This comment has been minimized.

Sign in to view

sharwell added the Blocked label Sep 24, 2019

Document dependencies, lazy cancellation, and parameter name

578e20e

sharwell force-pushed the task-chaining branch from a1a9139 to 578e20e Compare September 25, 2019 14:44

sharwell removed the Blocked label Sep 25, 2019

jasonmalinowski reviewed Oct 8, 2019

View reviewed changes

Merge remote-tracking branch 'dotnet/master' into task-chaining

589390a

sharwell requested a review from a team December 6, 2019 00:14

Provide a cancellation token when joining _allRenameLocationsTask

b062327

jasonmalinowski approved these changes Jan 10, 2020

View reviewed changes

Merge remote-tracking branch 'dotnet/master' into task-chaining

c273daf

sharwell mentioned this pull request Jan 10, 2020

InlineRenameSession mixes data and UI operations #40890

Closed

Document unintended UI dependency

d9ed55c

sharwell merged commit 0a0baef into dotnet:master Jan 10, 2020

sharwell deleted the task-chaining branch January 10, 2020 21:39

sharwell added this to the 16.5.P3 milestone Jan 10, 2020

Conversation

sharwell commented Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RikkiGibson commented Jun 6, 2019

Uh oh!

JoeRobich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

sharwell commented Sep 25, 2019

Uh oh!

RikkiGibson commented Oct 3, 2019

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sharwell commented Dec 31, 2019

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sharwell commented Mar 19, 2019 •

edited

Loading

sharwell Dec 6, 2019 •

edited

Loading