Explicitly check for TTL expiration when using Ping utility #65312
Conversation
ghost
assigned
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFixes issue #65065 For some reason, TTL expiration went through the same path as timout expiration. This PR adds code which explicitly checks if TTL occured.
|
| { | ||
| // TTL exceeded may have occured | ||
| output = await p.StandardOutput.ReadToEndAsync().ConfigureAwait(false); | ||
| if (output.Contains("Time to live exceeded", StringComparison.Ordinal)) |
There was a problem hiding this comment.
This is going to be problematic IMHO. This message may come out localized. I'm wondering if we should accept that we cannot really distinguish the cases with confidence..
cc: @filipnavara @tmds
There was a problem hiding this comment.
maybe not. It seems like we explicitly set LC_ALL but dependency on particular text string still worries me little bit.
There was a problem hiding this comment.
IMHO it's worth a try. We can check strings in the common implementations (FreeBSD, GNU Inetutils, macOS*). Worst case it will behave the same way as it does now, best case we can provide slightly more accurate result.
However, the code needs to do way more input checking than it does now. The .IndexOf calls may fail to find anything and it would throw ArgumentOutOfRangeException.
There was a problem hiding this comment.
I realize that the dependency is suboptimal, one of the reason I put up this PR is to check is to check if it gets a hit on CI (I tried both Arch and Ubuntu locally, so I have hopes)
Will add checking of the indices
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
CI Failures seem unrelated:
|
Fixes issue #65065
For some reason, TTL expiration went through the same path as timout expiration. This PR adds code which explicitly checks if TTL occured.