Skip to content

Update the task-based async APIs added to PowerShell to return a Task object directly #9079

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.

Sign up for GitHub

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

Merged
daxian-dbw merged 2 commits into from
Mar 7, 2019
Merged

Update the task-based async APIs added to PowerShell to return a Task object directly #9079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f68cfc8
Update the async APIs to directly return a Task without using async/a…
daxian-dbw Mar 6, 2019
3933ada
Update tests
daxian-dbw Mar 6, 2019
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
28 changes: 12 additions & 16 deletions src/System.Management.Automation/engine/hostifaces/PowerShell.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3083,10 +3083,8 @@ public IAsyncResult BeginInvoke<TInput, TOutput>(PSDataCollection<TInput> input,
/// <exception cref="ObjectDisposedException">
/// Object is disposed.
/// </exception>
public async Task<PSDataCollection<PSObject>> InvokeAsync()
{
return await Task<PSDataCollection<PSObject>>.Factory.FromAsync(BeginInvoke(), _endInvokeMethod).ConfigureAwait(false);
}
public Task<PSDataCollection<PSObject>> InvokeAsync()
=> Task<PSDataCollection<PSObject>>.Factory.FromAsync(BeginInvoke(), _endInvokeMethod);
Copy link
Collaborator

@rjmholt rjmholt Mar 6, 2019

Choose a reason for hiding this comment

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

The thing I'm curious about is that the documentation here says that the FromAsync overload taking an IAsyncResult rather than a Func<AsyncCallback, object, IAsyncResult> is slower.

I tried looking into this, and the doc written for wrapping the old API with the new one here, but I'm not quite sure what needs to be done to turn the BeginInvoke() method we have into the appropriate callback handler

Copy link
Collaborator

@rjmholt rjmholt Mar 6, 2019

Choose a reason for hiding this comment

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

Here's an SO post that touches on this question

Copy link
Collaborator

@rjmholt rjmholt Mar 6, 2019

Choose a reason for hiding this comment

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

Here's another one that has some more detail

Copy link
Collaborator

@rjmholt rjmholt Mar 6, 2019

Choose a reason for hiding this comment

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

Ah, after reading a bit more, it looks like the callback functionality would need to be supported by PowerShell itself and passed into a BeginInvoke() method we provide so that it can signal that the task is done.

I was proceeding under the impression that it might be possible to fudge that with a wrapping lambda based on the documentation's recommendation of "use this overload", but I'm guessing that's wrong.

Copy link
Member Author

@daxian-dbw daxian-dbw Mar 7, 2019

Choose a reason for hiding this comment

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

Yeah, our Begin/End implementation doesn't strictly conform to the APM contract.

Copy link
Collaborator

@iSazonov iSazonov Mar 7, 2019

Choose a reason for hiding this comment

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

Will improve?

Copy link
Member Author

@daxian-dbw daxian-dbw Mar 7, 2019

Choose a reason for hiding this comment

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

I don't see a reason to update the existing APM APIs.

Copy link
Collaborator

@iSazonov iSazonov Mar 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

There was not addressed comment #8056 (comment)

Copy link
Member Author

@daxian-dbw daxian-dbw Mar 7, 2019

Choose a reason for hiding this comment

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

I think we can defer opening that issue until we get some real feedback on those async APIs that request performance enhancement.
Bruce brought that up mainly because when it was discussed in the committee meeting, there was an ask about if we should have separate implementation for the task-based APIs.


/// <summary>
/// Invoke a PowerShell command asynchronously.
Expand Down Expand Up @@ -3126,8 +3124,8 @@ public async Task<PSDataCollection<PSObject>> InvokeAsync()
/// <exception cref="ObjectDisposedException">
/// Object is disposed.
/// </exception>
public async Task<PSDataCollection<PSObject>> InvokeAsync<T>(PSDataCollection<T> input)
=> await Task<PSDataCollection<PSObject>>.Factory.FromAsync(BeginInvoke<T>(input), _endInvokeMethod).ConfigureAwait(false);
public Task<PSDataCollection<PSObject>> InvokeAsync<T>(PSDataCollection<T> input)
=> Task<PSDataCollection<PSObject>>.Factory.FromAsync(BeginInvoke<T>(input), _endInvokeMethod);

/// <summary>
/// Invoke a PowerShell command asynchronously.
Expand Down Expand Up @@ -3177,8 +3175,8 @@ public async Task<PSDataCollection<PSObject>> InvokeAsync<T>(PSDataCollection<T>
/// <exception cref="ObjectDisposedException">
/// Object is disposed.
/// </exception>
public async Task<PSDataCollection<PSObject>> InvokeAsync<T>(PSDataCollection<T> input, PSInvocationSettings settings, AsyncCallback callback, object state)
=> await Task<PSDataCollection<PSObject>>.Factory.FromAsync(BeginInvoke<T>(input, settings, callback, state), _endInvokeMethod).ConfigureAwait(false);
public Task<PSDataCollection<PSObject>> InvokeAsync<T>(PSDataCollection<T> input, PSInvocationSettings settings, AsyncCallback callback, object state)
=> Task<PSDataCollection<PSObject>>.Factory.FromAsync(BeginInvoke<T>(input, settings, callback, state), _endInvokeMethod);

/// <summary>
/// Invoke a PowerShell command asynchronously.
Expand Down Expand Up @@ -3225,8 +3223,8 @@ public async Task<PSDataCollection<PSObject>> InvokeAsync<T>(PSDataCollection<T>
/// <exception cref="ObjectDisposedException">
/// Object is disposed.
/// </exception>
public async Task<PSDataCollection<PSObject>> InvokeAsync<TInput, TOutput>(PSDataCollection<TInput> input, PSDataCollection<TOutput> output)
=> await Task<PSDataCollection<PSObject>>.Factory.FromAsync(BeginInvoke<TInput, TOutput>(input, output), _endInvokeMethod).ConfigureAwait(false);
public Task<PSDataCollection<PSObject>> InvokeAsync<TInput, TOutput>(PSDataCollection<TInput> input, PSDataCollection<TOutput> output)
=> Task<PSDataCollection<PSObject>>.Factory.FromAsync(BeginInvoke<TInput, TOutput>(input, output), _endInvokeMethod);

/// <summary>
/// Invoke a PowerShell command asynchronously and collect
Expand Down Expand Up @@ -3284,8 +3282,8 @@ public async Task<PSDataCollection<PSObject>> InvokeAsync<TInput, TOutput>(PSDat
/// <exception cref="ObjectDisposedException">
/// Object is disposed.
/// </exception>
public async Task<PSDataCollection<PSObject>> InvokeAsync<TInput, TOutput>(PSDataCollection<TInput> input, PSDataCollection<TOutput> output, PSInvocationSettings settings, AsyncCallback callback, object state)
=> await Task<PSDataCollection<PSObject>>.Factory.FromAsync(BeginInvoke<TInput, TOutput>(input, output, settings, callback, state), _endInvokeMethod).ConfigureAwait(false);
public Task<PSDataCollection<PSObject>> InvokeAsync<TInput, TOutput>(PSDataCollection<TInput> input, PSDataCollection<TOutput> output, PSInvocationSettings settings, AsyncCallback callback, object state)
=> Task<PSDataCollection<PSObject>>.Factory.FromAsync(BeginInvoke<TInput, TOutput>(input, output, settings, callback, state), _endInvokeMethod);

/// <summary>
/// Begins a batch execution.
Expand Down Expand Up @@ -3808,10 +3806,8 @@ public void EndStop(IAsyncResult asyncResult)
/// <exception cref="ObjectDisposedException">
/// Object is disposed.
/// </exception>
public async Task StopAsync(AsyncCallback callback, object state)
{
await Task.Factory.FromAsync(BeginStop(callback, state), _endStopMethod).ConfigureAwait(false);
}
public Task StopAsync(AsyncCallback callback, object state)
=> Task.Factory.FromAsync(BeginStop(callback, state), _endStopMethod);

#endregion

Expand Down
15 changes: 8 additions & 7 deletions test/powershell/engine/Api/TaskBasedAsyncPowerShellAPI.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,15 +84,16 @@ try {
It 'cannot invoke a single script asynchronously in a runspace that has not been opened' {
$rs = [runspacefactory]::CreateRunspace()
$ps = [powershell]::Create($rs)

try {
$r = $ps.AddScript('@(1..10).foreach{Start-Sleep -Milliseconds 500}').InvokeAsync()
# This test is designed to fail. You cannot invoke PowerShell asynchronously
# in a runspace that has not been opened.
$r.IsFaulted | Should -Be $true
$r.Exception -is [System.AggregateException] | Should -Be $true
$r.Exception.InnerException -is [System.Management.Automation.Runspaces.InvalidRunspaceStateException] | Should -Be $true
$r.Exception.InnerException.CurrentState | Should -Be 'BeforeOpen'
$r.Exception.InnerException.ExpectedState | Should -Be 'Opened'
$err = { $ps.AddScript('1+1').InvokeAsync() } | Should -Throw -ErrorId "InvalidRunspaceStateException" -PassThru

$err.Exception | Should -BeOfType "System.Management.Automation.MethodInvocationException"
$err.Exception.InnerException | Should -BeOfType "System.Management.Automation.Runspaces.InvalidRunspaceStateException"
$err.Exception.InnerException.CurrentState | Should -Be 'BeforeOpen'
$err.Exception.InnerException.ExpectedState | Should -Be 'Opened'
} finally {
$ps.Dispose()
$rs.Dispose()
Expand All @@ -117,7 +118,7 @@ try {
# This test is designed to fail. You cannot invoke PowerShell asynchronously
# in the current runspace because nested PowerShell instances cannot be
# invoked asynchronously
$err = { InvokeAsyncHelper -PowerShell $ps -Wait } | Should -Throw -ErrorId 'AggregateException' -PassThru
$err = { $ps.AddScript('1+1').InvokeAsync() } | Should -Throw -ErrorId 'PSInvalidOperationException' -PassThru
GetInnerErrorId -Exception $err.Exception | Should -Be 'InvalidOperation'
} finally {
$ps.Dispose()
Expand Down