FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Make Test-Connection always use the default synchronization context for sending ping requests by daxian-dbw · Pull Request #11517 · PowerShell/PowerShell · GitHub

Make Test-Connection always use the default synchronization context for sending ping requests - #11517

Merged
Ilya (iSazonov) merged 4 commits into
PowerShell:masterfrom
daxian-dbw:test-connection
Jan 22, 2020
Merged

Make Test-Connection always use the default synchronization context for sending ping requests#11517
Ilya (iSazonov) merged 4 commits into
PowerShell:masterfrom
daxian-dbw:test-connection

Conversation

Dongbo Wang (daxian-dbw) commented Jan 8, 2020
edited
Loading

Copy link
Copy Markdown
Member

PR Summary

Fix #11418

When using Ping.SendAsync with a callback, the callback will always be invoked in the synchronization context of the pipeline thread. When the current synchronization context of the thread is null, the default context is used which is the threadpool.
In case the pipeline thread's synchronization context is set (for example, by constructing a WindowsForm object), the callback will be scheduled to run on the pipeline thread, which will result in a dead lock.
The fix is to use Ping.SendPingAsync.

Note about SendAsync and PingCompletedEventArgs

Note that, when using Ping.SendAsync, _pingCompleteArgs.Cancelled is never set to true when the task is cancelled by Ctrl+c. Instead, _pingCompleteArgs.Error is set with a PingException whose InnerException is TaskCanceledException.
I think this is a bug in the Ping.SendAsync API, but didn't spend the time to dig into it.

PR Checklist

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Seems sound to me. This looks a lot nicer!

Thanks for fixing this! 😊 💖

ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jan 8, 2020

Rain Sallow (/u/ta11ow) (vexx32) commented Jan 8, 2020
edited
Loading

Copy link
Copy Markdown
Collaborator

Dongbo Wang (@daxian-dbw) does this allow Ctrl+C to cancel the ping? The documentation for SendAsyncCancel() mentions that SendAsyncCancel() is meant to cancel async requests submitted with SendAsync() 🤔

Downloaded the build artifact, and it seems to cancel, but the task doesn't return until the predefined timeout either way... though it does stop. Weird. So I guess it sort of does, but it won't cause the task to return right away. Odd. I think it was already doing that before this change, though, so it must be something that the API is doing internally, a shared code path somewhere, most likely.

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jan 8, 2020

Copy link
Copy Markdown
Member Author

Rain Sallow (/u/ta11ow) (@vexx32) SendAsync is actually just a thin wrapper over SendPingAsync, see the implementation here. So SendAsyncCancel() should work for SendPingAsync as well. I did set breakpoint in the code and see the PingException being thrown with a TaskCanceledException inner exception when pressing ctrl+c. Yes, I guess the cancellation doesn't happen right away.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, nice catch on the inner exception 🙂

One comment

Ilya (iSazonov) commented Jan 8, 2020
edited
Loading

Copy link
Copy Markdown
Collaborator

When using Ping.SendAsync with a callback, the callback will always be invoked in the synchronization context of the pipeline thread.

Is it EAP behavior? I ask because SendAsync() creates an async task with ConfigureAwait(false)
https://source.dot.net/#System.Net.Ping/System/Net/NetworkInformation/Ping.cs,281
https://source.dot.net/#System.Net.Ping/System/Net/NetworkInformation/Ping.cs,328
https://source.dot.net/#System.Net.Ping/System/Net/NetworkInformation/Ping.cs,341

Also I should point may be related #11420 (comment)
There Test-Connection, traceroute and the same SendCancellablePing(). Test on Azure takes up to 40 sec! Setting Timeout parameter causes hang.
I guess it is the same issue like this.

.Net Core uses ConfigureAwait(false) everywhere. If there is something that blocking current thread I guess it is either Core bug or by-design in EAP.

Rain Sallow (/u/ta11ow) (vexx32) commented Jan 8, 2020
edited
Loading

Copy link
Copy Markdown
Collaborator

Ilya (@iSazonov) yeah traceroutes take a while even on Windows, but are especially bad on Unix, since all Unix Ping APIs do nothing except report TimedOut for any non-success response (TtlExpired is not a status that Unix APIs will return under any circumstances I've found, so traceroute on Unix is currently rather busted and always waits the full Timeout value before returning). It may be worth simply skipping the traceroute tests under Unix until we get better functionality from .NET Core if we aren't already.

Copy link
Copy Markdown
Contributor

Ilya (@iSazonov)

Is it EAP behavior? I ask because SendAsync() creates an async task with ConfigureAwait(false)

The EAP version of the API is just a wrapper around the TAP version. That wrapper uses AsyncOperationManager, which looks at the SynchronizationContext for the current thread. The TAP version doesn't use the AsyncOperationManager, instead it just uses the native API directly with a callback that sets a TaskCompletionSource.

If there is something that blocking current thread I guess it is either Core bug or by-design in EAP.

The problem is with certain sync contexts, particularly the one set by the Form constructor, it causes a deadlock. You could argue that it's by design because Form isn't being used how it's intended (e.g. from PowerShell) but still worth fixing.

Copy link
Copy Markdown
Collaborator

Patrick Meinecke (@SeeminglyScience) Thanks! I see. Not clear is it a bug or by-design for EAP.

Copy link
Copy Markdown
Contributor

I would say that it's by design that the EAP version honors the current synch context, PowerShell just doesn't play well with the windows forms sync context implementation specifically. Or really any external sync context I would guess.

Dongbo Wang (daxian-dbw) commented Jan 8, 2020
edited
Loading

Copy link
Copy Markdown
Member Author

I agree that it's by design for the EAP version. It's the right thing to invoke the callback in the current synchronization context. Library APIs themselves should always use the default sync context, but when interacting with the user code, it's the user's decision if the current sync context should be honored (though the EAP version API doesn't let you choose :)).

But I guess there is a bug in the EAP version API, where the PingCompletedEventArgs.Cancelled never gets set to true when the task is cancelled by SendAsyncCancel().

Copy link
Copy Markdown
Member Author

Steve Lee (@SteveL-MSFT) Please take another look when you have time.
Ilya (@iSazonov) The PR is stuck with WIP check, but I don't know why 😕

Copy link
Copy Markdown
Collaborator

Seems like the bot isn't adding the status either way... interesting. Perhaps try editing the PR title / summary a bit and see if that triggers whatever webhook the bot may be using?

Copy link
Copy Markdown
Member Author

Rain Sallow (/u/ta11ow) (@vexx32) Yeah, I will edit the title a bit to see if that helps.

Copy link
Copy Markdown
Member Author

PoshChan-Bot (@PoshChan) Please retry windows

Copy link
Copy Markdown
Collaborator

Dongbo Wang (@daxian-dbw), successfully started retry of PowerShell-CI-Windows

Ilya (iSazonov) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

With one minor comment.

Copy link
Copy Markdown
Member Author

Steve Lee (@SteveL-MSFT) Can you please update your review? Thanks

Copy link
Copy Markdown
Member Author

Steve Lee (@SteveL-MSFT) A gentle ping. Please take another look when you get a chance.

Copy link
Copy Markdown
Member Author

Steve Lee (@SteveL-MSFT) ping again 👋 😃

Copy link
Copy Markdown
Collaborator

I vote to have the fix in 7.0 servicing because users use often WinForms and now Out-ConsoleGridView.

Copy link
Copy Markdown
Member Author

Moved the PR to 7.0.x-servicing-consider.

Copy link
Copy Markdown

🎉v7.1.0-preview.1 has been released which incorporates this pull request.:tada:

Handy links:

Copy link
Copy Markdown

🎉v7.0.1 has been released which incorporates this pull request.:tada:

Handy links:

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test-Connection 'hangs' after New-Object System.Windows.Forms.Form

8 participants


Back | FazBrowse Home | New Git URL