Skip to content

Conversation

@jjw24
Copy link
Member

@jjw24 jjw24 commented Mar 16, 2021

@jjw24 jjw24 added the bug Something isn't working label Mar 16, 2021
@jjw24 jjw24 requested a review from taooceros March 16, 2021 19:36
@jjw24 jjw24 self-assigned this Mar 16, 2021
@jjw24 jjw24 enabled auto-merge March 16, 2021 19:36
try
{
for (var index = 0; index < results.Count; index++)
await Task.Run(() =>
Copy link
Member

Choose a reason for hiding this comment

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

I would say that we don't need the Task.Run because we don't need to filter result in another thread. It is sync code so using a Task.Run won't create better performance. We just need to add the token.Throw here, and this method don't need to be asynchronous.
The OperationCancelledException is supposed to be catched by PluginManager, so no need to add another try catch here. I try to do that in WebSearch plugin because I have encoutered debugging latency (the latency only appears when debugger attach, and I haven't found a great way to resolve that), but seems that simply catching the result won't solve that issue. Therefore, no need 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.

The OperationCancelledException is supposed to be catched by PluginManager, so no need to add another try catch here.

There is a try catch in IndexSearch class, how come that needs it?

Copy link
Member

Choose a reason for hiding this comment

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

I would say no need for that, but since there already contains a try catch for ODBC exception, I add it. We can remove it to keep code cleaner.

Copy link
Member Author

@jjw24 jjw24 Mar 17, 2021

Choose a reason for hiding this comment

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

Sounds good, I will do that. So overall, there is no need to catch token cancellation thrown exception in plugins, because it will be caught by the PluginManager, correct?

Copy link
Member

Choose a reason for hiding this comment

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

Correct. Exception thrown is by design to avoid result return, so no need to catch them inside any plugin.

@jjw24 jjw24 changed the title Add ConfigureWait and cancellation token Add ConfigureWait and cancellation token & remove unnecessary OperationCanceledException Mar 17, 2021
@jjw24
Copy link
Member Author

jjw24 commented Mar 17, 2021

@taooceros done, please review

Copy link
Member

@taooceros taooceros left a comment

Choose a reason for hiding this comment

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

Nice Work!

@jjw24 jjw24 merged commit 4dcc279 into dev Mar 18, 2021
@jjw24 jjw24 deleted the fix_windowsindex_exclude branch March 18, 2021 03:48
Copy link
Member

@taooceros taooceros left a comment

Choose a reason for hiding this comment

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

Nice Work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants