Skip to content

Do not throw exception if LanguageDebugInfoService.GetDataTipInfoAsync do not run to completion#1926

Merged
dsyme merged 2 commits intodotnet:masterfrom
vasily-kirichenko:fix-debugging-tooltips
Dec 5, 2016
Merged

Do not throw exception if LanguageDebugInfoService.GetDataTipInfoAsync do not run to completion#1926
dsyme merged 2 commits intodotnet:masterfrom
vasily-kirichenko:fix-debugging-tooltips

Conversation

@vasily-kirichenko
Copy link
Contributor

@vasily-kirichenko vasily-kirichenko commented Dec 3, 2016

This fixes #1915

Basically, it turned out we should not (re) throw exceptions if the task has not run to completion. Previously, we ran the async computation with StartAsyncAsTask https://github.com/Microsoft/visualfsharp/blob/master/vsintegration/src/FSharp.Editor/CommonRoslynHelpers.fs#L31

let GetCompletedTaskResult(task: Task<'TResult>) =
        if task.Status = TaskStatus.RanToCompletion then
            task.Result
        else
            Assert.Exception(task.Exception.GetBaseException())
            raise(task.Exception.GetBaseException())

let StartAsyncAsTask cancellationToken computation =
        Async.StartAsTask(computation, TaskCreationOptions.None, cancellationToken)
             .ContinueWith(GetCompletedTaskResult, cancellationToken)

I'm not sure I understand why we have that else branch where we throw the base exception. Could anybody explain this? Should we remove it as it may cause other code misbehavior?

@vasily-kirichenko vasily-kirichenko mentioned this pull request Dec 3, 2016
Closed
@dsyme
Copy link
Contributor

dsyme commented Dec 3, 2016

@vasily-kirichenko Yes I'm pretty certain we should remove it, and just use the same helper everywhere.

@dsyme
Copy link
Contributor

dsyme commented Dec 3, 2016 •
edited
Loading

@vasily-kirichenko Please merge with master, thanks

I think we should also review the 2-3 commits where @OmarTawfik and I added the logic for StartAsyncAsTask. Maybe there is a need to throw the exception (e.g. OperationCanceledException) though it seems unlikely

I suppose if we're going to implicity throw exceptions away we should be more explicit about it, and add an assert (except for OperationCanceledException) so we know it's happening in debug mode.

@vasily-kirichenko
Copy link
Contributor Author

vasily-kirichenko commented Dec 4, 2016

@dsyme Done. I've just removed ContinueWith and use async's try..with instead.

@dsyme dsyme merged commit 2068fb5 into dotnet:master Dec 5, 2016
@cartermp
Copy link
Contributor

cartermp commented Dec 5, 2016

🎉

nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
@vasily-kirichenko @dsyme
Do not throw exception if LanguageDebugInfoService.GetDataTipInfoAsyn…
ac3b11c 
…c do not run to completion (dotnet#1926)

* do not throw exception if LanguageDebugInfoService.GetDataTipInfoAsync do not run to completion

* CommonRoslynHelpers.StartAsyncAsTask catches exceptions and transform them to Assert.Exception instead of reraising
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Debugger tooltips do not work on large solutions

4 participants

@vasily-kirichenko @dsyme @cartermp @msftclas