-
-
Notifications
You must be signed in to change notification settings - Fork 2
Handle root query client lifetimes #471
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,3 @@ | ||
| using System.Runtime.CompilerServices; | ||
|
|
||
| [assembly: InternalsVisibleTo("DnsClientX.Tests")] |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -13,6 +13,10 @@ namespace DnsClientX { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Provides synchronous and asynchronous methods for performing DNS lookups. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// </remarks> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public partial class ClientX { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| internal static Func<ClientX> RootClientFactory { get; set; } = () => new ClientX(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| internal static Func<ClientX, string, DnsRecordType, CancellationToken, Task<DnsResponse>>? RootResolveOverride { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Sends a DNS query for a specific record type to a DNS server. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// This method allows you to specify the DNS endpoint from a predefined list of endpoints. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -91,14 +95,27 @@ public static DnsResponse QueryDnsSync(string name, DnsRecordType recordType, Dn | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// <returns>A task that represents the asynchronous operation. The task result contains the DNS response.</returns> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public static async Task<DnsResponse[]> QueryDns(string[] name, DnsRecordType recordType, DnsEndpoint dnsEndpoint = DnsEndpoint.System, DnsSelectionStrategy dnsSelectionStrategy = DnsSelectionStrategy.First, int timeOutMilliseconds = Configuration.DefaultTimeout, bool retryOnTransient = true, int maxRetries = 3, int retryDelayMs = 200, bool requestDnsSec = false, bool validateDnsSec = false, bool typedRecords = false, bool parseTypedTxtRecords = false, CancellationToken cancellationToken = default) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
95
to
96
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// <returns>A task that represents the asynchronous operation. The task result contains the DNS response.</returns> | |
| public static async Task<DnsResponse[]> QueryDns(string[] name, DnsRecordType recordType, DnsEndpoint dnsEndpoint = DnsEndpoint.System, DnsSelectionStrategy dnsSelectionStrategy = DnsSelectionStrategy.First, int timeOutMilliseconds = Configuration.DefaultTimeout, bool retryOnTransient = true, int maxRetries = 3, int retryDelayMs = 200, bool requestDnsSec = false, bool validateDnsSec = false, bool typedRecords = false, bool parseTypedTxtRecords = false, CancellationToken cancellationToken = default) { | |
| /// <returns>A task that represents the asynchronous operation. The task result contains the DNS response.</returns> | |
| /// <exception cref="ArgumentNullException">Thrown if <paramref name="name"/> is null.</exception> | |
| /// <remarks> | |
| /// If <paramref name="name"/> is an empty array, the method returns an empty <see cref="DnsResponse"/> array. | |
| /// </remarks> | |
| public static async Task<DnsResponse[]> QueryDns(string[] name, DnsRecordType recordType, DnsEndpoint dnsEndpoint = DnsEndpoint.System, DnsSelectionStrategy dnsSelectionStrategy = DnsSelectionStrategy.First, int timeOutMilliseconds = Configuration.DefaultTimeout, bool retryOnTransient = true, int maxRetries = 3, int retryDelayMs = 200, bool requestDnsSec = false, bool validateDnsSec = false, bool typedRecords = false, bool parseTypedTxtRecords = false, CancellationToken cancellationToken = default) { | |
| if (name == null) { | |
| throw new ArgumentNullException(nameof(name)); | |
| } | |
| if (name.Length == 0) { | |
| return Array.Empty<DnsResponse>(); | |
| } |
Copilot
AI
Dec 9, 2025
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.
[nitpick] Consider checking cancellationToken.IsCancellationRequested before the LINQ query (line 100) to avoid creating and disposing clients unnecessarily when the operation is already cancelled.
| if (dnsEndpoint == DnsEndpoint.RootServer) { | |
| if (dnsEndpoint == DnsEndpoint.RootServer) { | |
| if (cancellationToken.IsCancellationRequested) { | |
| return await Task.FromCanceled<DnsResponse[]>(cancellationToken).ConfigureAwait(false); | |
| } |
Copilot
AI
Dec 9, 2025
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.
This variable is manually disposed in a finally block - consider a C# using statement as a preferable resource management technique.
| try { | |
| var tasks = name.Select(n => { | |
| var client = RootClientFactory(); | |
| clients.Add(client); | |
| if (cancellationToken.IsCancellationRequested) { | |
| return Task.FromCanceled<DnsResponse>(cancellationToken); | |
| } | |
| var resolver = RootResolveOverride; | |
| return resolver != null | |
| ? resolver(client, n, recordType, cancellationToken) | |
| : client.ResolveFromRoot(n, recordType, cancellationToken: cancellationToken); | |
| }).ToArray(); | |
| var tasks = name.Select(n => { | |
| var client = RootClientFactory(); | |
| clients.Add(client); | |
| if (cancellationToken.IsCancellationRequested) { | |
| return Task.FromCanceled<DnsResponse>(cancellationToken); | |
| } | |
| var resolver = RootResolveOverride; | |
| return resolver != null | |
| ? resolver(client, n, recordType, cancellationToken) | |
| : client.ResolveFromRoot(n, recordType, cancellationToken: cancellationToken); | |
| }).ToArray(); | |
| try { |
Copilot
AI
Dec 9, 2025
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 any Dispose() call throws an exception in the finally block, it will suppress exceptions from Task.WhenAll(). Consider wrapping the disposal loop in a try-catch to prevent disposal exceptions from masking task failures:
} finally {
foreach (var client in clients) {
try {
client.Dispose();
} catch {
// Log or ignore disposal failures
}
}
}| client.Dispose(); | |
| try { | |
| client.Dispose(); | |
| } catch { | |
| // Log or ignore disposal failures | |
| } |
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.
The XML documentation comment should be placed before the
[Fact]attribute, not after it. The standard convention is to place documentation comments immediately before attributes.