New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve Task usage #1172
Improve Task usage #1172
Conversation
@@ -639,267 +639,262 @@ public Task<RunspaceHandle> GetRunspaceHandleAsync(CancellationToken cancellatio | |||
errorMessages, | |||
executionOptions)).ConfigureAwait(false); | |||
} | |||
else | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you hide whitespace changes here, you'll see I just got rid of the else
. Been wanting to do that for a while.
src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM just a couple nits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
RunWriter, | ||
CancellationToken.None, // Inner method will manage cancellation | ||
TaskCreationOptions.LongRunning | TaskCreationOptions.DenyChildAttach, | ||
TaskScheduler.Default); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this never exits, I'd just throw it in a Thread
like you mentioned.
…hellContextService.cs Co-Authored-By: Tyler James Leonhardt <tylerl0706@gmail.com>
Here is an overview of what got changed by this pull request: Issues
======
+ Solved 3
See the complete overview on Codacy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Makes the following changes:
Task.Factory.StartNew
invocations toTask.Run
to simplify the child attach semantics (invoking tasks won't hang around for new threads to finish now, since in the places where we use it, it seems we really wanted to kick off a new thread)async
/await
from methods that just do some setup and run another taskTaskOptions.LongRunning
hint to the logger thread so that it's less likely to impact the thread pool. The other option here is to use a newThread
and bypass the thread pool altogether. Happy to do either (but this one represents a smaller change).