Skip to content

Do not cancel tasks which requires clearing existing results #3577

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 3 commits into from
Closed

Do not cancel tasks which requires clearing existing results #3577

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ee52d7
Do not cancel tasks which is for clearing existing results
Jack251970 May 23, 2025
445ca9a
Do not pass token to make sure it is handled
Jack251970 May 23, 2025
634e829
Improve code comments
Jack251970 May 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions Flow.Launcher/ViewModel/MainViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,8 @@
await Task.Delay(20);
while (channelReader.TryRead(out var item))
{
if (!item.Token.IsCancellationRequested)
// If the update task is not canceled or requires clearing existing results, add it to the queue
if (!item.Token.IsCancellationRequested || item.shouldClearExistingResults)
queue[item.ID] = item;
}

Expand Down Expand Up @@ -1437,8 +1438,9 @@
_lastQuery = query;
_previousIsHomeQuery = currentIsHomeQuery;

// If this update task is for clearing existing results, do not pass token to make sure it is handled
if (!_resultsUpdateChannelWriter.TryWrite(new ResultsForUpdate(resultsCopy, plugin.Metadata, query,
token, reSelect, shouldClearExistingResults)))
shouldClearExistingResults ? default : token, reSelect, shouldClearExistingResults)))
{
App.API.LogError(ClassName, "Unable to add item to Result Update Queue");
}
Expand All @@ -1460,8 +1462,9 @@
_lastQuery = query;
_previousIsHomeQuery = currentIsHomeQuery;

// If this update task is for clearing existing results, do not pass token to make sure it is handled
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What scenario could have this occur?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If one task which needs to clear existing results, we should make sure it is handled in Results. So we need to ignore its origin token here.

Copy link
Member

@jjw24 jjw24 May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the original code if a task is cancelled, it wouldn't get to this part of the code right

Copy link
Member Author

@Jack251970 Jack251970 May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see the update flow above in desc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still in the process to update the results though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm show me what exactly do you mean.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually in this PR we are just trying to handle the case where token is cancelled but require clearing results still, so we can just capture this where token is cancelled and require clearing are true then return empty result set in that NewResults method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jjw24 Please go ahead with this. I find I am unable to do that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually in this PR we are just trying to handle the case where token is cancelled but require clearing results still, so we can just capture this where token is cancelled and require clearing are true then return empty result set in that NewResults method.

Feel free to change anything you would like

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am little unfamiliar with this clear logic and the only thing I can change is about handling with token cancellation.

if (!_resultsUpdateChannelWriter.TryWrite(new ResultsForUpdate(results, _historyMetadata, query,
token, reSelect, shouldClearExistingResults)))
shouldClearExistingResults ? default : token, reSelect, shouldClearExistingResults)))
{
App.API.LogError(ClassName, "Unable to add item to Result Update Queue");
}
Expand Down Expand Up @@ -1842,7 +1845,7 @@
VisibilityChanged?.Invoke(this, new VisibilityChangedEventArgs { IsVisible = false });
}

#pragma warning restore VSTHRD100 // Avoid async void methods

Check warning on line 1848 in Flow.Launcher/ViewModel/MainViewModel.cs

View workflow job for this annotation

GitHub Actions / Check Spelling 

`VSTHRD` is not a recognized word. (unrecognized-spelling)

/// <summary>
/// Save history, user selected records and top most records
Expand All @@ -1861,12 +1864,21 @@
{
if (!resultsForUpdates.Any())
return;

CancellationToken token;

try
{
// Don't know why sometimes even resultsForUpdates is empty, the method won't return;
token = resultsForUpdates.Select(r => r.Token).Distinct().SingleOrDefault();
// If there are any update tasks for clearing existing results, we need to set token to default so that it will not be cancelled
if (resultsForUpdates.Any(r => r.shouldClearExistingResults))
{
token = default;
}
else
{
// Don't know why sometimes even resultsForUpdates is empty, the method won't return;
token = resultsForUpdates.Select(r => r.Token).Distinct().SingleOrDefault();
}
}
#if DEBUG
catch
Expand Down
Loading