Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CancellationToken-accepting overloads for SendPingAsync #72338

Merged
merged 4 commits into from
Aug 30, 2022

Conversation

madelson
Copy link
Contributor

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

Fix #67260

…work).

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

Fix dotnet#67260
@dotnet-issue-labeler
Copy link

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 ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 17, 2022
@ghost
Copy link

ghost commented Jul 17, 2022

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: -

Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

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

Generally looks good, besides a few questions.

@rzikm rzikm assigned madelson and unassigned rzikm Aug 8, 2022
@madelson
Copy link
Contributor Author

madelson commented Aug 8, 2022

@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
Copy link
Member

rzikm commented Aug 8, 2022

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

madelson commented Aug 9, 2022

@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?

@madelson madelson requested review from stephentoub and rzikm August 15, 2022 01:00
Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

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

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

using Process pingProcess = GetPingProcess(address, buffer, timeout, options);
pingProcess.Start();

var timedOutOrCanceled = false;
Copy link
Member

Choose a reason for hiding this comment

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

var => bool

}
catch (TimeoutException)
catch when (timeoutOrCancellationToken.IsCancellationRequested)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

@@ -142,6 +149,8 @@ private void InternalDispose()
_status = Disposed;
}

_timeoutOrCancellationSource?.Dispose();
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

I feel like this comment is just wrong.

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

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);
Copy link
Member

Choose a reason for hiding this comment

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

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

{
CancellationToken timeoutOrCancellationToken = _timeoutOrCancellationSource!.Token;
Copy link
Member

Choose a reason for hiding this comment

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

Move this down into the try block?

@@ -83,6 +85,7 @@ private void CheckStart()
currentStatus = _status;
if (currentStatus == Free)
{
_timeoutOrCancellationSource ??= new();
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

IPAddress address = await getAddress(getAddressArg, _timeoutOrCancellationSource!.Token).ConfigureAwait(false);

Task<PingReply> pingTask = SendPingAsyncCore(address, buffer, timeout, options);
_timeoutOrCancellationSource.CancelAfter(timeout);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

@madelson madelson requested a review from stephentoub August 18, 2022 11:04
@rzikm
Copy link
Member

rzikm commented Aug 24, 2022

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

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

@rzikm rzikm left a comment

Choose a reason for hiding this comment

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

LGTM

@rzikm rzikm requested a review from stephentoub August 29, 2022 07:39
@rzikm
Copy link
Member

rzikm commented Aug 30, 2022

Thanks for the contribution!

@rzikm rzikm merged commit e00bd40 into dotnet:main Aug 30, 2022
@karelz karelz added this to the 8.0.0 milestone Sep 3, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Add Ping.SendPingAsync overloads with CancellationToken
4 participants