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 default timeout mention. #5162

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add default timeout mention. #5162

wants to merge 1 commit into from

Conversation

NN---
Copy link

@NN--- NN--- commented Dec 10, 2020

Summary

Describe your changes here.

Fixes #Issue_Number (if available)

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@opbld33
Copy link

opbld33 commented Dec 10, 2020

Docs Build status updates of commit e8782bb:

✅ Validation status: passed

File Status Preview URL Details
xml/System.Net.NetworkInformation/Ping.xml ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@gewarren gewarren requested a review from a team December 17, 2020 04:01
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

What's driving this addition? Is there any opened issue to which is this change related?

@@ -64,7 +64,9 @@
Network topology can determine whether <xref:System.Net.NetworkInformation.Ping> can successfully contact a remote host. The presence and configuration of proxies, network address translation (NAT) equipment, or firewalls can prevent <xref:System.Net.NetworkInformation.Ping> from succeeding. A successful <xref:System.Net.NetworkInformation.Ping> indicates only that the remote host can be reached on the network; the presence of higher level services (such as a Web server) on the remote host is not guaranteed.

This class provides functionality similar to the Ping.exe command line tool. The <xref:System.Net.NetworkInformation.Ping.Send%2A> and <xref:System.Net.NetworkInformation.Ping.SendAsync%2A> methods send an Internet Control Message Protocol (ICMP) echo request message to a remote computer and waits for an ICMP echo reply message from that computer. For a detailed description of ICMP messages, see RFC 792, available at [https://www.ietf.org](https://www.ietf.org/).


The default timeout when timeout is not specified is 5 seconds similar to Ping.exe command line tool timeout.
Copy link
Member

Choose a reason for hiding this comment

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

Could you please link a source of this information? Is it also true on *nixes?

Copy link
Member

Choose a reason for hiding this comment

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

I see no reason to add reference to ping.exe for public API doc. stating the default should be sufficient IMHO.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't mean link it here, I was curious where the 5 seconds came from.

@NN---
Copy link
Author

NN--- commented Dec 17, 2020

@ManickaP
Copy link
Member

This is not true though:

var p = new Ping();
var sw = Stopwatch.StartNew();
var x = p.Send(new IPAddress(new byte[]{192, 168, 0, 42}));
Console.WriteLine(sw.Elapsed);
Console.WriteLine(x.Status);

However, it will timeout in 5 seconds if ran under sudo, because that will give it permission to send ICMP directly.

@wfurt could you please chime in here?

Also I digged up the original issue in runtime repo: dotnet/runtime#45888 It would be nice to include it in the description with a small text explaining why is this change happening next time.

Base automatically changed from master to main March 5, 2021 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants