| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
Seems sound to me. This looks a lot nicer!
Thanks for fixing this! 😊 💖
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM, nice catch on the inner exception 🙂
One comment
Sorry, something went wrong.
Is it EAP behavior? I ask because SendAsync() creates an async task with ConfigureAwait(false) Also I should point may be related #11420 (comment) .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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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.
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. |
Sorry, something went wrong.
|
Patrick Meinecke (@SeeminglyScience) Thanks! I see. Not clear is it a bug or by-design for EAP. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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(). |
Sorry, something went wrong.
|
Steve Lee (@SteveL-MSFT) Please take another look when you have time. |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
Rain Sallow (/u/ta11ow) (@vexx32) Yeah, I will edit the title a bit to see if that helps. |
Sorry, something went wrong.
|
PoshChan-Bot (@PoshChan) Please retry windows |
Sorry, something went wrong.
|
Dongbo Wang (@daxian-dbw), successfully started retry of PowerShell-CI-Windows |
Sorry, something went wrong.
There was a problem hiding this comment.
With one minor comment.
Sorry, something went wrong.
|
Steve Lee (@SteveL-MSFT) Can you please update your review? Thanks |
Sorry, something went wrong.
|
Steve Lee (@SteveL-MSFT) A gentle ping. Please take another look when you get a chance. |
Sorry, something went wrong.
|
Steve Lee (@SteveL-MSFT) ping again 👋 😃 |
Sorry, something went wrong.
|
I vote to have the fix in 7.0 servicing because users use often WinForms and now Out-ConsoleGridView. |
Sorry, something went wrong.
|
Moved the PR to 7.0.x-servicing-consider. |
Sorry, something went wrong.
|
🎉v7.1.0-preview.1 has been released which incorporates this pull request.:tada: Handy links: |
Sorry, something went wrong.
|
🎉v7.0.1 has been released which incorporates this pull request.:tada: Handy links: |
Sorry, something went wrong.
…or sending ping requests (PowerShell#11517)
…or sending ping requests (PowerShell#11517)
| Back | FazBrowse Home | New Git URL |
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