-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Gather more diagnostics, and prepare skipping flaky DNS tests #36072
Closed
Closed
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
c241f1a
NameResolutionTestHelper
antonfirsov 22c7929
add NameResolutionTestHelper.cs
antonfirsov 68982c1
move NameResolutionTestHelper
antonfirsov 1ffdd4e
cleanup, and apply EnsureNameToAddressWorks
antonfirsov 5f4ee8f
fix Windows build
antonfirsov d32e04c
cleanup messy auto-refactor code
antonfirsov cc85528
fix comment
antonfirsov f505e0c
Merge branch 'master' into af/dns-tests-3
antonfirsov 9e42819
use gethostbyname
antonfirsov dde5bf9
move NameResolutionTestHelper
antonfirsov 1cab6a7
handle hostName == ""
antonfirsov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
99 changes: 99 additions & 0 deletions
99
src/libraries/Common/tests/System/Net/Sockets/NameResolutionTestHelper.Unix.cs
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
#nullable enable | ||
using System.IO; | ||
using System.Runtime.InteropServices; | ||
using System.Text; | ||
using Xunit.Abstractions; | ||
|
||
namespace System.Net.NameResolution.Tests | ||
{ | ||
internal static class NameResolutionTestHelper | ||
{ | ||
/// <summary> | ||
/// Check if name resolution (getaddrinfo) at OS level. | ||
/// In case of failure we may force the tests fail, so we can gather diagnostics from the CI environment. | ||
/// After collecting enough information we shall alter call sites to use throwOnFailure:false, | ||
/// since the failure does not indicate an error within the NCL product code. | ||
/// </summary> | ||
public static unsafe bool EnsureNameToAddressWorks(string hostName, ITestOutputHelper? testOutput, bool throwOnFailure) | ||
{ | ||
addrinfo hint = default; | ||
hint.ai_family = 0; // AF_UNSPEC | ||
hint.ai_flags = 2; // AI_CANONNAME | ||
hint.ai_socktype = 1; // SOCK_STREAM; | ||
|
||
addrinfo* res = default; | ||
|
||
int err = getaddrinfo(hostName, null, &hint, &res); | ||
freeaddrinfo(res); | ||
|
||
if (err != 0) | ||
{ | ||
string failureInfo = | ||
$"Failed to resolve '{hostName}'! getaddrinfo error: {err}{Environment.NewLine}{LogUnixInfo()}"; | ||
testOutput?.WriteLine(failureInfo); | ||
if (throwOnFailure) | ||
{ | ||
throw new Exception(failureInfo); | ||
} | ||
|
||
return false; | ||
} | ||
|
||
return true; | ||
} | ||
|
||
private static string LogUnixInfo() | ||
{ | ||
StringBuilder bld = new StringBuilder(); | ||
bld.AppendLine($"Dns.GetHostName() == {Dns.GetHostName()}"); | ||
bld.AppendLine("--- /etc/hosts ---"); | ||
|
||
string etcHosts; | ||
try | ||
{ | ||
etcHosts = File.ReadAllText("/etc/hosts"); | ||
} | ||
catch (Exception ex) | ||
{ | ||
etcHosts = $"Failed to retrieve /etc/hosts: {ex.Message}"; | ||
} | ||
|
||
string resolvConf; | ||
try | ||
{ | ||
resolvConf = File.ReadAllText("/etc/resolv.conf"); | ||
} | ||
catch (Exception ex) | ||
{ | ||
resolvConf = $"Failed to retrieve /etc/resolv.conf: {ex.Message}"; | ||
} | ||
|
||
bld.AppendLine(etcHosts); | ||
bld.AppendLine("--- /etc/resolv.conf ---"); | ||
bld.AppendLine(resolvConf); | ||
bld.AppendLine("------"); | ||
return bld.ToString(); | ||
} | ||
|
||
#pragma warning disable 169 | ||
[StructLayout(LayoutKind.Sequential)] | ||
unsafe struct addrinfo | ||
{ | ||
public int ai_flags; | ||
public int ai_family; | ||
public int ai_socktype; | ||
int ai_protocol; | ||
uint ai_addrlen; | ||
UIntPtr ai_addr; | ||
UIntPtr ai_canonname; | ||
addrinfo* ai_next; | ||
}; | ||
#pragma warning restore 169 | ||
|
||
[DllImport("libc")] | ||
private static extern unsafe int getaddrinfo(string node, string service, addrinfo* hints, addrinfo** res); | ||
|
||
[DllImport("libc")] | ||
private static extern unsafe void freeaddrinfo(addrinfo* res); | ||
} | ||
} |
14 changes: 14 additions & 0 deletions
14
src/libraries/Common/tests/System/Net/Sockets/NameResolutionTestHelper.Windows.cs
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
#nullable enable | ||
using Xunit.Abstractions; | ||
|
||
namespace System.Net.NameResolution.Tests | ||
{ | ||
internal static class NameResolutionTestHelper | ||
{ | ||
public static unsafe bool EnsureNameToAddressWorks(string hostName, ITestOutputHelper? testOutput, bool throwOnFailure = true) | ||
{ | ||
// NOP, since we do not expect DNS failures on Windows. | ||
return true; | ||
} | ||
} | ||
} |
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this P/Invoking is unlikely to be portable across various Unix implementations. It's why we use C shims as an intermediary for the production source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was suggesting to use
gethostbyname()
as it takes string returns NULL or result so it should be easy to check and be portable IMHO. We chat about it with @antonfirsov and the goal is to check if system can resolve it self without depending on production code we are trying to test. We do not need to process the answer in any detailed way. We could still move it to Interop to make it more portable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wfurt the main reason for choosing
freeaddrinfo
was because ofgethostbyname
being marked obsolete. Also:freeaddrinfo
provides a trivial way to also get the error code, which I find useful.@stephentoub while this is true in general, I don't see any reason to worry about it in this particular case. The
addrinfo
structure seems binary compatible across unixes, and the lines responsible for the query in our own PAL code are also portable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've seen numerous cases where the actual layout differed in header files from what was shown in the man pages, including but not limited to when targeting 32-bit vs 64-bit.
Those are compiled by the C compiler, which sees the actual header files. That's the primary reason the native shims exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also agree that we should not p/invoke directly but only use native C shims.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I understand that @antonfirsov. But it gets you out of the structure business. Since it has pointers, the size will differ. socklen_t seems to be fixed to 32bit but I'm not sure if that is mandatory for al OSes.
Shim as @stephentoub suggested would be portable path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I understand the issue now.
From the various options I see, the simplest and safest one seems to be the use of
gethostbyname
as @wfurt suggested. Looks like there are several existing cases where we P/Invoke into similar simple methods in test code. Is there any strong counter-argument against usinggethostbyname
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you mean the:
function, and we treat the return as an opaque pointer, e.g.
I'm ok with that.