Add CancellationToken-accepting overloads for SendPingAsync#72338

madelson · 2022-07-17T13:49:33Z

Provides "true" cancellation for async ping methods using either a token or
the existing SendAsyncCancel() API.

…work). Provides "true" cancellation for async ping methods using either a token or the existing SendAsyncCancel() API. Fix dotnet#67260

ghost · 2022-07-17T13:49:39Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2022-07-17T13:49:47Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Provides "true" cancellation for async ping methods using either a token or
the existing SendAsyncCancel() API.

Fix #67260

Author:	madelson
Assignees:	-
Labels:	`area-System.Net`, `new-api-needs-documentation`
Milestone:	-

src/libraries/System.Net.Ping/ref/System.Net.Ping.cs

src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.PingUtility.cs

src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.cs

rzikm

Generally looks good, besides a few questions.

src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.PingUtility.cs

src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.cs

madelson · 2022-08-08T11:50:41Z

@rzikm I thought I had asked about this in another comment, but I can't find it now. One callout is that this does not implement true cancellation on Windows because Windows pings use a native function which wasn't obviously cancelable from my read of the docs (let me know if I missed something). Maybe this is something that can be handled by CancelIOEx?

One other option I considered was changing the Windows ping to use raw sockets when possible (as is done for Unix). However, I'm not sure of the pros/cons there.

Thoughts?

rzikm · 2022-08-08T13:03:41Z

One callout is that this does not implement true cancellation on Windows because Windows pings use a native function which wasn't obviously cancelable from my read of the docs (let me know if I missed something). Maybe this is something that can be handled by CancelIOEx?

It would be nice to have consistent behavior between platforms if it can be achieved, I did a quick test and CancelIO didn't work (returned INVALID_HANDLE). Not sure if using raw ICMP sockets is viable, maybe @antonfirsov will know.

stephentoub · 2022-08-09T11:42:08Z

One callout is that this does not implement true cancellation on Windows because Windows pings use a native function which wasn't obviously cancelable from my read of the docs

I've not read the change yet. What do you do in this case? If it's just not as cancelable, e.g. once the native call is made the cancellation token is ignored, that's ok. If instead cancellation triggers the returned task to complete even while the native operation continues, that's not ok.

stephentoub · 2022-08-09T11:43:04Z

One other option I considered was changing the Windows ping to use raw sockets when possible (as is done for Unix). However, I'm not sure of the pros/cons there.

IIRC, creating raw sockets requires elevated/admin privileges. If that's the case, we can't do that.

madelson · 2022-08-09T11:53:22Z

@stephentoub

I've not read the change yet. What do you do in this case? If it's just not as cancelable, e.g. once the native call is made the cancellation token is ignored, that's ok. If instead cancellation triggers the returned task to complete even while the native operation continues, that's not ok.

It is much closer to the former than the latter but not exactly. Before this change, we already had a (mostly non-functional) version of cancellation via the Ping.SendAsyncCancel() method which would just set the _canceled flag. When the async operation completed, we'd convert to a canceled result based on the task. I didn't change this code at all for the Windows branch. The only change in the behavior is that previously cancellations were getting wrapped as PingException and now cancellations flow through without wrapping so that all cancellations manifest in the same way.

IIRC, creating raw sockets requires elevated/admin privileges. If that's the case, we can't do that.

In Unix, we use the raw socket version conditionally based on whether we have permissions. I was proposing that we do the same thing for Windows. Maybe you're just saying that people running in admin mode is so rare that having this fork isn't worth it? Or maybe you're saying that the permission check for Unix doesn't work on Windows?

rzikm

LGTM now, but I would like @stephentoub to have a look as well in case I missed something.

stephentoub · 2022-08-16T01:14:27Z

src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.PingUtility.cs

+            using Process pingProcess = GetPingProcess(address, buffer, timeout, options);
+            pingProcess.Start();
+
+            var timedOutOrCanceled = false;


var => bool

stephentoub · 2022-08-16T01:16:25Z

src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.PingUtility.cs

+                    stdout = await pingProcess.StandardOutput.ReadToEndAsync(timeoutOrCancellationToken).ConfigureAwait(false);
                }
-                catch (TimeoutException)
+                catch when (timeoutOrCancellationToken.IsCancellationRequested)


We don't want to only catch OperationCanceledException? If the wait failed with an unrelated exception at appx the same time that cancellation was requested, we'll end up eating that exception and treating it as a timeout?

Happy to make this change, just a few questions:

We don't want to only catch OperationCanceledException

Were you imagining

catch (OperationCanceledException) { ... }

or

catch (OperationCanceledException) when (timeoutOrCancellationToken.IsCancellationRequested) { ... }

If we go with the former it seems that there is some (maybe negligible) risk that something in the try block throws OCE for some reaons other than a passed-in cancellation token (a real-world example is HttpClient throwing TaskCanceledException when it times out).

Similarly, by catching only OCE is seems there is some risk that something we are calling will wrap the OCE (a real-world example is SendPingAsync() prior to this PR.

Do either of these scenarios concern you?

If the wait failed with an unrelated exception at appx the same time that cancellation was requested, we'll end up eating that exception and treating it as a timeout

In that case, does it matter? We have a race condition where multiple things happened that should have aborted the ping and we are correctly informing the caller about one of those things.

Were you imagining

The latter.

In that case, does it matter? We have a race condition where multiple things happened that should have aborted the ping and we are correctly informing the caller about one of those things.

When this situation typically arises elsewhere, we make the same argument, but in those cases there's the added complication that the cancellation could have actually caused the other exception, and then it's actually preferable to show the cancelation one because the other is fake in a sense. Here, though, I don't think that's the case, in which case we generally want to prioritize real failures over manufactured ones from timeouts and cancellation, as they typically convey better information about the state of the system.

stephentoub · 2022-08-16T01:19:56Z

src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.cs

                _status = Disposed;
            }

+            _timeoutOrCancellationSource?.Dispose();


Dispose doesn't Cancel. Do you want to also Cancel? I'm just wondering about the comment on this method which suggests it does so.

Interesting. I feel like this comment is just wrong. The way the method used to and still works is that if called while a ping request is in progress it sets _disposeRequested to true and then returns. Then when the ping request completes it calls Finish which completes the disposal.

It would be interesting if this comment implied that what InternalDisposeCore does (disposing the handles) can actually safely cancel outstanding Windows ping requests, but given that the old .NET Framework version seems to have the same behavior I'm not convinced.

Do you agree? Should I just remove the comment? Should we change the code to start canceling the token once we've requested disposal to hurry outstanding requests along?

I feel like this comment is just wrong.

Looks like you're right. You can just delete it.

stephentoub · 2022-08-16T01:23:22Z

src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.cs

-                IPAddress[] addresses = await Dns.GetHostAddressesAsync(hostNameOrAddress).ConfigureAwait(false);
-                Task<PingReply> pingReplyTask = SendPingAsyncCore(addresses[0], buffer, timeout, options);
-                return await pingReplyTask.ConfigureAwait(false);
+                using CancellationTokenRegistration _ = cancellationToken.Register(static state => ((Ping)state!).SetCanceled(), this);


It's unlikely to make a material difference, but it looks like this could use UnsafeRegister instead of Register.

stephentoub · 2022-08-16T01:27:33Z

src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.RawSocket.cs

-        private static async Task<PingReply> SendIcmpEchoRequestOverRawSocketAsync(IPAddress address, byte[] buffer, int timeout, PingOptions? options)
+        private async Task<PingReply> SendIcmpEchoRequestOverRawSocketAsync(IPAddress address, byte[] buffer, int timeout, PingOptions? options)
        {
+            CancellationToken timeoutOrCancellationToken = _timeoutOrCancellationSource!.Token;


Move this down into the try block?

stephentoub · 2022-08-16T01:28:11Z

src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.cs

                currentStatus = _status;
                if (currentStatus == Free)
                {
+                    _timeoutOrCancellationSource ??= new();


You should be able to mark this method as [MemberNotNull(nameof(_timeoutOrCancellationSource))], which should let you remove at least one if not more !s on use of that field.

Do you mean other methods? This is the one that lazily initializes the CancellationTokenSource (Finish then nulls it out if it has become canceled).

I mean putting that attribute on this method should let you remove the ! from other methods that call this one.

stephentoub · 2022-08-16T01:32:58Z

src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.cs

+                IPAddress address = await getAddress(getAddressArg, _timeoutOrCancellationSource!.Token).ConfigureAwait(false);
+
+                Task<PingReply> pingTask = SendPingAsyncCore(address, buffer, timeout, options);
+                _timeoutOrCancellationSource.CancelAfter(timeout);


Doing this here means the timeout won't apply to the GetHostAddressesAsync inside getAddress. Why is that ok / desirable? I'm also unclear as to why this is being done like this after the SendPingAsyncCore. Normally this kind of ordering would be to avoid a race condition, but that doesn't apply here.

Both of these are attempts to maintain the existing behavior / be true to the documentation:

The old code does not apply the timeout to Dns.GetHostAddressesAsync (see https://source.dot.net/#System.Net.Ping/System/Net/NetworkInformation/Ping.cs,646).

The documentation for the timeout parameter says: "The maximum number of milliseconds (after sending the echo message) to wait for the ICMP echo reply message".

I feel like the timeout is supposed to be inherent to the ping request, rather than being a general timeout for the entire operation. In contrast, I think the CancellationToken is the way for the caller to abort the operation whenever they want. Thoughts?

I'll defer to @dotnet/ncl on the desired behavior here. I expect it was documented that way because there was literally no way before for the Dns operations to be timed out. But there is now. The question then is whether we want to update the timeout to apply to the whole operation or just to the waiting for the ICMP echo reply message; I'm ok either way.

I've not looked; are we consistent with the timeout only applying to the ICMP echo reply across all implementations?

Personally, I feel like it is better to keep the timeout only for the ICMP echo reply. As noted, users can use the CancellationToken to cancel the operation x ms from the start of it, and if we make the timeout parameter cover the DNS resolution as well, users may get different results depending on whether DNS was fast enough or was/wasn't cached.

madelson · 2022-08-17T11:03:51Z

Thanks for all the feedback @rzikm @stephentoub . At this point I'm just waiting on resolutions to a few open threads:

rzikm · 2022-08-24T12:12:53Z

Looks like there is just one thing to address #72338 (comment) and we will be good to go, right? @madelson?

rzikm

LGTM

rzikm · 2022-08-30T16:13:39Z

Thanks for the contribution!

Add CancellationToken-accepting overloads for SendPingAsync (initial …

1fc763f

…work). Provides "true" cancellation for async ping methods using either a token or the existing SendAsyncCancel() API. Fix dotnet#67260

ghost added area-System.Net new-api-needs-documentation labels Jul 17, 2022

ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 17, 2022

madelson commented Jul 17, 2022

View reviewed changes

src/libraries/System.Net.Ping/ref/System.Net.Ping.cs Show resolved Hide resolved

madelson commented Jul 17, 2022

View reviewed changes

src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.PingUtility.cs Show resolved Hide resolved

madelson commented Jul 17, 2022

View reviewed changes

src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.cs Show resolved Hide resolved

madelson commented Jul 17, 2022

View reviewed changes

src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.cs Show resolved Hide resolved

madelson commented Jul 17, 2022

View reviewed changes

src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.cs Outdated Show resolved Hide resolved

madelson commented Jul 17, 2022

View reviewed changes

src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.cs Show resolved Hide resolved

madelson marked this pull request as ready for review July 17, 2022 16:59

runfoapp bot mentioned this pull request Jul 19, 2022

System.Net.Http.Functional.Tests.SyncHttpHandler_DiagnosticsTest.SendAsync_SuppressedGlobalStaticPropagationEnvVar failing in CI #72428

Closed

karelz requested a review from rzikm August 2, 2022 16:13

rzikm self-assigned this Aug 8, 2022

rzikm reviewed Aug 8, 2022

View reviewed changes

rzikm assigned madelson and unassigned rzikm Aug 8, 2022

Address feedback from dotnet#72338

f3360f7

madelson requested review from rzikm and stephentoub August 15, 2022 01:00

rzikm approved these changes Aug 15, 2022

View reviewed changes

stephentoub reviewed Aug 16, 2022

View reviewed changes

Feedback from dotnet#72338

317e50f

madelson requested a review from stephentoub August 18, 2022 11:04

Address feedback from dotnet#72338 (comment)

431fa08

madelson requested review from rzikm and removed request for stephentoub August 26, 2022 12:30

rzikm approved these changes Aug 29, 2022

View reviewed changes

rzikm requested a review from stephentoub August 29, 2022 07:39

rzikm merged commit e00bd40 into dotnet:main Aug 30, 2022

karelz added this to the 8.0.0 milestone Sep 3, 2022

ghost locked as resolved and limited conversation to collaborators Oct 3, 2022

Comments

Conversation

madelson commented Jul 17, 2022

Uh oh!

ghost commented Jul 17, 2022

Uh oh!

ghost commented Jul 17, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rzikm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

madelson commented Aug 8, 2022

Uh oh!

rzikm commented Aug 8, 2022

Uh oh!

stephentoub commented Aug 9, 2022

Uh oh!

stephentoub commented Aug 9, 2022

Uh oh!

madelson commented Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rzikm 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!

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!

Choose a reason for hiding this comment

Uh oh!

madelson commented Aug 17, 2022

Uh oh!

rzikm commented Aug 24, 2022

Uh oh!

rzikm left a comment

Choose a reason for hiding this comment

Uh oh!

rzikm commented Aug 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

madelson commented Aug 9, 2022 •

edited

Loading