Cancel Dashboard loading when navigating away #3587
Conversation
krschau
requested review from
dhoehna,
guimafelipe,
vineeththomasalex,
dkbennett,
EricJohnson327,
lauren-ciha and
AmelBawa-msft
| @@ -289,6 +300,12 @@ private async Task RestorePinnedWidgetsAsync(ComSafeWidget[] hostWidgets) | |||
| // append it at the end. If a position is missing, just show the next widget in order. | |||
| foreach (var widget in hostWidgets) | |||
| { | |||
| if (cancellationToken.IsCancellationRequested) | |||
There was a problem hiding this comment.
Curious what's the crash about? Assuming some resources are disposed? Nonetheless, this likely greatly reduces the crash but may not fully eliminate it depending on the concurrency execution, for example if IsCancellationRequested was set right after the if statement. I see that you've linked some related tasks to further strengthen this 👍
There was a problem hiding this comment.
It's System.InvalidOperationException: Collection was modified; enumeration operation may not execute. Specifically, in OnUnloaded we do PinnedWidgets.Clear() and it was modifying PinnedWidgets. So if OnLoaded was still running, the iterators over PinnedWidgets inside RestorePinnedWidgetsAsync() were now invalid and throwing.
It's specifically the case where we leave Dashboard while it's still loading.
There was a problem hiding this comment.
Gotcha. For that case, we need to ensure shared resources are properly locked (issue linked in PR description). We likely don't need the token if that was updated, but I can still see it useful for optimization purposes.
There was a problem hiding this comment.
Yeah, I considered just locking but I'd really prefer to bail early if possible since it's really a lot of work that gets us nothing. I looked into locking as well, but since it should go everywhere I think that deserves its own PR (for the issue linked)
| { | ||
| var comSafeWidgetDefinitions = await ComSafeHelpers.GetAllOrderedComSafeWidgetDefinitions(ViewModel.WidgetHostingService); | ||
| foreach (var comSafeWidgetDefinition in comSafeWidgetDefinitions) | ||
| { | ||
| if (cancellationToken.IsCancellationRequested) |
There was a problem hiding this comment.
I would strongly recommend changing the cancellation token usage pattern to assume it throws and to throw when cancellation is requested. Many methods that support cancellation will use CancellationToken.ThrowIfCancellationRequested(), and if this cancellation token is eventually passed to a method outside Dev Home that doesn't quietly return as this code does and instead throws, we'll have a potential unhandled exception. So I would recommend changing this pattern to use ThrowIfCancellationRequested() and assume OperationCanceledException should be caught for this scenario. We should be then free to pass the cancellation token to any APIs that use one, even outside Dev Home and can handle any throw and cleanup as appropriate with other exception handling.
There was a problem hiding this comment.
Good catch, forgot that it can throw as well 😬
|
I've started hitting NullReferenceException crashes on |
Summary of the pull request
Navigating away from the Dashboard will clear out PinnedWidgets. However, if the widgets were still being loaded, clearing PinnedWidgets while iterating through it will cause a crash. When we navigate away, we should cancel the loading so we don't hit this scenario.
We do still have work item #1215 to further support concurrency, but this will fix the issue in #3558, which is more likely to be hit than other concurrency scenarios.
References and relevant issues
Detailed description of the pull request / Additional comments
Validation steps performed
PR checklist