GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Update the task-based async APIs added to PowerShell to return a Task object directly by daxian-dbw · Pull Request #9079 · PowerShell/PowerShell

Conversation

Dongbo Wang (daxian-dbw) commented Mar 6, 2019 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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 Kirk Munro (@KirkMunro)

PR Context

PR Checklist

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Change is not breaking
Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
User-facing changes
- Not Applicable
- OR
- Documentation needed
  - Issue filed:
Testing - New and feature
- N/A or can only be tested interactively
- OR
- Make sure you've added a new test if existing tests do not effectively test the code changed
  - Add [feature] to your commit messages if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

KirkMunro reacted with thumbs up emoji

All reactions

👍 1 reaction


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

f68cfc8

…wait

Dongbo Wang (daxian-dbw) self-assigned this

Mar 6, 2019

Dongbo Wang (daxian-dbw) requested review from Steve Lee (SteveL-MSFT) and Ilya (iSazonov)

March 6, 2019 18:39

Dongbo Wang (daxian-dbw) requested a review from Bruce Payette (BrucePay) as a code owner

March 6, 2019 18:39

Dongbo Wang (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

Ilya (iSazonov) reviewed

Mar 6, 2019

View reviewed changes

Comment thread

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

Uh oh!

There was an error while loading. Please reload this page.


          Update tests

3933ada

Dongbo Wang (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

Rob Holt (rjmholt) approved these changes

Mar 6, 2019

View reviewed changes

Rob Holt (rjmholt) left a comment

Copy link

Copy Markdown

Collaborator

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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);

Rob Holt (rjmholt) Mar 6, 2019

Copy link

Copy Markdown

Collaborator

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Rob Holt (rjmholt) Mar 6, 2019

Copy link

Copy Markdown

Collaborator

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.

Here's an SO post that touches on this question

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Rob Holt (rjmholt) Mar 6, 2019

Copy link

Copy Markdown

Collaborator

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.

Here's another one that has some more detail

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Rob Holt (rjmholt) Mar 6, 2019

Copy link

Copy Markdown

Collaborator

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dongbo Wang (daxian-dbw) Mar 7, 2019

Copy link

Copy Markdown

Member Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) Mar 7, 2019

Copy link

Copy Markdown

Collaborator

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.

Will improve?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dongbo Wang (daxian-dbw) Mar 7, 2019

Copy link

Copy Markdown

Member Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) Mar 7, 2019 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

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.

There was not addressed comment #8056 (comment)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dongbo Wang (daxian-dbw) Mar 7, 2019

Copy link

Copy Markdown

Member Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

iSazonov reacted with thumbs up emoji

All reactions

👍 1 reaction

Ilya (iSazonov) commented Mar 7, 2019

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Dongbo Wang (daxian-dbw) commented Mar 7, 2019 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

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. Aditya Patwardhan (@adityapatwardhan) can you please confirm?

iSazonov reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ilya (iSazonov) approved these changes

Mar 7, 2019

View reviewed changes

Dongbo Wang (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

Dongbo Wang (daxian-dbw) merged commit 50f3e7f into PowerShell:master

Mar 7, 2019

Dongbo Wang (daxian-dbw) deleted the asyncAPI branch

March 7, 2019 19:24

Travis Plunk (TravisEz13) added this to the 6.2.0 milestone

Mar 7, 2019

Travis Plunk (TravisEz13) added a commit to TravisEz13/PowerShell that referenced this pull request

Mar 7, 2019


          Update the task-based async APIs added to PowerShell to return a Task…

c3ab27a

… 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

Travis Plunk (TravisEz13) pushed a commit that referenced this pull request

Mar 13, 2019


          Update the task-based async APIs added to PowerShell to return a Task…

fefe563

… 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.

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Update the task-based async APIs added to PowerShell to return a Task…

02d09e7

… 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.

Reviewers

Ilya (iSazonov)

iSazonov approved these changes

Steve Lee (SteveL-MSFT)

Awaiting requested review from SteveL-MSFT

Bruce Payette (BrucePay)

Awaiting requested review from BrucePay

+1 more reviewer

Rob Holt (rjmholt)

rjmholt approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Dongbo Wang (daxian-dbw)

Labels

CL-Engine

Indicates that a PR should be marked as an engine change in the Change Log

Projects

None yet

Milestone

6.2.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Dongbo Wang (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!

Rob Holt (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!

Ilya (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!

Ilya (iSazonov) commented Mar 7, 2019

Uh oh!

Dongbo Wang (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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Dongbo Wang (daxian-dbw) commented Mar 6, 2019 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Ilya (iSazonov) Mar 7, 2019 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Dongbo Wang (daxian-dbw) commented Mar 7, 2019 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.