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

daxian-dbw · 2019-03-06T18:39:39Z

PR Summary

Update the task-based async APIs added to PowerShell to not use the aysnc/await keywords, but to return a Task object directly.
There is nothing to continue on after the Task.Factory.FromAsync call in those methods, so there is not need to use aysnc and await keywords, which turns the method into a state machine class.

The behavior of the API is slightly different comparing with before this change.
But note that, the behavior when calling the async APIs with await is the same -- exception will be thrown at await.

Before this change:

Calling InvokeAsync() without await will always return a task even if the BeginInvoke call, which gets evaluated before calling FromAsync, throws an exception.
This is because with async/await, this method is written by compiler with a state machine, where all exceptions are caught can passed to the task that is going to be returned.

After this change:

Calling InvokeAsync() without await will throw exception if the BeginInovke call throws exception. So it fails early.

/cc @KirkMunro

PR Context

PR Checklist

…wait

src/System.Management.Automation/engine/hostifaces/PowerShell.cs

rjmholt

This change makes sense to me. The InvokeAsync method itself shouldn't be async, since it just wraps another asynchronous invocation. Therefore it should synchronously pass the handle back to the caller. Everything looks good -- just want to discuss the recommended overload to see if we can improve our usage of that.

rjmholt · 2019-03-06T23:06:05Z

src/System.Management.Automation/engine/hostifaces/PowerShell.cs

-            return await Task<PSDataCollection<PSObject>>.Factory.FromAsync(BeginInvoke(), _endInvokeMethod).ConfigureAwait(false);
-        }
+        public Task<PSDataCollection<PSObject>> InvokeAsync()
+            => Task<PSDataCollection<PSObject>>.Factory.FromAsync(BeginInvoke(), _endInvokeMethod);


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

Here's an SO post that touches on this question

Here's another one that has some more detail

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.

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

Will improve?

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

There was not addressed comment #8056 (comment)

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.

iSazonov · 2019-03-07T03:46:43Z

@daxian-dbw With the PR we get a possibility to use the methods in scripts, yes? So we need to update related doc issue.

daxian-dbw · 2019-03-07T04:51:43Z

Scripts can use those new async APIs. They are tested in Pester. APIs are documented by its xml doc comments, right? I guess nothing needs to be done in the powershell-doc repo. @adityapatwardhan can you please confirm?

… object directly (PowerShell#9079) Update the task-based async APIs added to PowerShell to not use the `aysnc/await` keywords, but to return a `Task` object directly. There is nothing to continue on after the `Task.Factory.FromAsync` call in those methods, so there is not need to use `aysnc` and `await` keywords, which turns the method into a state machine class unnecessarily. # Conflicts: # src/System.Management.Automation/engine/hostifaces/PowerShell.cs # test/powershell/engine/Api/TaskBasedAsyncPowerShellAPI.Tests.ps1

… object directly (#9079) Update the task-based async APIs added to PowerShell to not use the `aysnc/await` keywords, but to return a `Task` object directly. There is nothing to continue on after the `Task.Factory.FromAsync` call in those methods, so there is not need to use `aysnc` and `await` keywords, which turns the method into a state machine class unnecessarily.

Update the async APIs to directly return a Task without using async/a…

f68cfc8

…wait

daxian-dbw self-assigned this Mar 6, 2019

daxian-dbw requested review from SteveL-MSFT and iSazonov March 6, 2019 18:39

daxian-dbw requested a review from BrucePay as a code owner March 6, 2019 18:39

daxian-dbw changed the title ~~Update the task-based async APIs added to PowerShell to return a Task object directly~~ WIP: Update the task-based async APIs added to PowerShell to return a Task object directly Mar 6, 2019

iSazonov reviewed Mar 6, 2019

View reviewed changes

src/System.Management.Automation/engine/hostifaces/PowerShell.cs Show resolved Hide resolved

Update tests

3933ada

daxian-dbw changed the title ~~WIP: Update the task-based async APIs added to PowerShell to return a Task object directly~~ Update the task-based async APIs added to PowerShell to return a Task object directly Mar 6, 2019

rjmholt approved these changes Mar 6, 2019

View reviewed changes

iSazonov approved these changes Mar 7, 2019

View reviewed changes

daxian-dbw added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Mar 7, 2019

daxian-dbw merged commit 50f3e7f into PowerShell:master Mar 7, 2019

daxian-dbw deleted the asyncAPI branch March 7, 2019 19:24

TravisEz13 added this to the 6.2.0 milestone Mar 7, 2019

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

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

Uh oh!

Conversation

daxian-dbw commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Before this change:

After this change:

PR Context

PR Checklist

Uh oh!

Uh oh!

rjmholt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Mar 7, 2019

Uh oh!

daxian-dbw commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

daxian-dbw commented Mar 6, 2019 •

edited

Loading

iSazonov Mar 7, 2019 •

edited

Loading

daxian-dbw commented Mar 7, 2019 •

edited

Loading