Update Ping.Windows.cs for async task handling #122054
Open
+19
−26
Conversation
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
Change TaskCompletionSource initialization to use RunContinuationsAsynchronously option and remove unnecessary sourceAddr.Clear() call.
dotnet-policy-service
bot
added
the
community-contribution
huoyaoyuan
reviewed
Nov 29, 2025
src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Windows.cs
Show resolved
Hide resolved
src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Windows.cs
Outdated
Show resolved
Hide resolved
Member
|
What is this PR addressing? |
stephentoub
added
the
NO-MERGE
Clear the sourceAddr span before sending the echo request.
Author
|
Waiting to all Performance Optimization. |
Member
How is this an optimization? If anything, it's most likely a regression, as it's forcibly adding an additional thread pool hop. |
Refactor unmanaged memory handling for async ICMP echo.
Author
|
deadlock Example: public async Task CheckNetworkConnectivity()
{
var ping = new Ping();
var tasks = new List<Task<PingReply>>();
for (int i = 0; i < 500; i++)
{
tasks.Add(ping.SendPingAsync("192.168.1.1"));
}
var results = await Task.WhenAll(tasks);
} |
src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Windows.cs
Outdated
Show resolved
Hide resolved
Refactor Ping reply handling to improve early return on failure and optimize data extraction.
MihaZupan
reviewed
Nov 30, 2025
src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Windows.cs
Outdated
Show resolved
Hide resolved
Removed the CopyDataFromUnmanagedMemory method to simplify code.
huoyaoyuan
reviewed
Nov 30, 2025
Member
huoyaoyuan
left a comment
huoyaoyuan
left a comment
There was a problem hiding this comment.
Please do not make random changes just for changing. The content of the pull request looks like the result of telling AI to "optimize some thing". There is little or no real benefit, if not negative.
src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Windows.cs
Outdated
Show resolved
Hide resolved
Comment on lines
+353
to
359
| static byte[] CreateBuffer(IntPtr data, int dataSize) | ||
| { | ||
| rtt = 0; | ||
| options = null; | ||
| buffer = Array.Empty<byte>(); | ||
| Span<byte> bufferSpan = stackalloc byte[dataSize]; | ||
| Marshal.Copy(data, bufferSpan, 0, dataSize); | ||
|
|
||
| return bufferSpan.ToArray(); | ||
| } |
Member
There was a problem hiding this comment.
This method is pure regression. It's still allocating a new array, and copying the data twice with a temporary span.
| PingOptions? options; | ||
| byte[] buffer; | ||
| var (rtt, options, buffer) = ipStatus == IPStatus.Success | ||
| ? (reply.roundTripTime, |
Member
There was a problem hiding this comment.
Using ?: here is much less readable.
Comment on lines
+312
to
+313
| Span<byte> destination = new Span<byte>(_requestBuffer.DangerousGetHandle().ToPointer(), buffer.Length); | ||
| buffer.CopyTo(destination); |
Member
There was a problem hiding this comment.
While the change is valid, we may do this better with the SafeLocalAllocHandle type itself, as a part of the reduce unsafe theme.
This was referenced Nov 30, 2025
Open
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change
SetUnmanagedStructuresvoid to useSpan <byte>and RedoCreatePingReplyFromIcmpEchoReplyvoid to Performance Optimization.