fallback to GetAddrInfoW if GetAddrInfoExW fails by wfurt · Pull Request #45816 · dotnet/runtime

wfurt · 2020-12-09T05:19:57Z

While root cause why GetAddrInfoExW() is failing from impersonificated context is still investigated this change should make Async DNS working again on Windows when running from impersonificated context.

We do not have infrastructure for automated authenticatiobn tests but I run the repro code manually on my Windows 10 and it works now eg. both explicit DNS lookup and HttpClient Async fetch works.

fixes #29935

ghost · 2020-12-09T05:20:01Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

While root cause why GetAddrInfoExW() is failing from impersonificated context is still investigated this change should make Async DNS working again on Windows when running from impersonificated context.

We do not have infrastructure for automated authenticatiobn tests but I run the repro code manually on my Windows 10 and it works now eg. both explicit DNS lookup and HttpClient Async fetch works.

Author:	wfurt
Assignees:	-
Labels:	`area-System.Net`
Milestone:	-

CarnaViire · 2020-12-09T13:43:48Z

Also, is it possible to add a test for these changes?

scalablecory

have we talked to Windows team to see if there's a way we can keep using async here?

wfurt · 2020-12-09T18:01:51Z

Also, is it possible to add a test for these changes?

no, not really. This was part of the Enterprise-auth project but it got de-prioritized.
In order to test this we need secondary Windows account can credentials for it so the test can impersonififcate - that is hard to maintain within current test framework.

Co-authored-by: Cory Nelson <phrosty@gmail.com>

…olutionPal.Windows.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>

wfurt · 2020-12-11T21:56:35Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2020-12-11T21:56:50Z

Azure Pipelines successfully started running 1 pipeline(s).

MihaZupan · 2020-12-15T07:22:05Z

-                return result;
+                if (NameResolutionPal.TryGetAddrInfoAsync(hostName, justAddresses, addressFamily, cancellationToken, out t))
+                {
+                    t.ContinueWith((resultTask) => {


We should be returning the Task returned by ContinueWith, or the ActivityId following the Dns resolution wouldn't reflect the Stop event.
This was the issue with the original Telemetry instrumentation of Dns, pointed out by @ManickaP in #41670 (comment) (and fixed in #42188).

Yep, this will break the telemetry once again.

I had different change where telemetry test was failing. With this on, telemetry tests are still passing. The log still be edited but I'm not sire about the ID. If we return the ContinueWith, we will need to copy result (or failure). I will look into it.

The tests will only validate that the Start/Stop are logged with the same ActivityId.

The issue here appears when you have another Start/Stop pair following the name resolution:

HttpRequestStart starts with ID /1/

NameResolutionStart starts with nested ID /1/1/

NameResolutionStop correctly sees /1/1/ and updates it to /1/.

SocketConnectStart starts on the same async context as NameResolutionStart, but it never observes the change of the ID back to /1/, so it starts with a nested ID /1/1/1

It will appear as if the Socket connect is an operation nested under the NameResolution Start/Stop, when in reality it's only following it.

I re-factor the code and the activity ID is right as far as I can tell as well as running impersonificated still works.

I went back to the await path as the ContinueWith was getting complicated more than I wanted. The only downside is that we will miss time of the call it self.
I feel it probably does not matter comparing duration of the actual resolution.
We could probably start the timer upfront but I'm not sure if that is worth of the complication.

after chatting with @MihaZupan , I decided to fix the stopwatch as well.
This should be ready for another review.

Telemetry here LGTM

…_29935

stephentoub · 2021-01-04T19:50:31Z

-                // instead of calling the synchronous version in the ThreadPool.
+                    // If the OS supports it and 'hostName' is not an IP Address, resolve the name asynchronously
+                    // instead of calling the synchronous version in the ThreadPool.
+                    // If it fails, we will fall-back to ThreadPool as well.


Suggested change

// If it fails, we will fall-back to ThreadPool as well.

// If it fails, we will fall back to ThreadPool as well.

stephentoub · 2021-01-08T22:34:21Z

+            {
+                // WSATRY_AGAIN indicates possible problem with reachability according to docs.
+                // However, if servers are really unreachable, we would still get IOPending here
+                // and final result would be posted via overlapped IO.


I'm not understanding the above comment.

WSATRY_AGAIN is valued error code for failures other than we see in the impersonification case. What I'm trying to say here is based on my testing and understanding, the other WSATRY_AGAIN would not be synchronous failure.
In such cases, we should return transient failure to caller rather than re-trying on thread-pool IMHO.

stephentoub · 2021-01-08T22:36:52Z

            {
-                result = await ((Task<T>)NameResolutionPal.GetAddrInfoAsync(hostName, justAddresses, addressFamily, cancellationToken)).ConfigureAwait(false);
-                return result;
+                //return new Tuple<bool, Task>(true, (Task)InternalGetAddrInfoWithTelemetryAsync(task, hostName));


wfurt · 2021-01-12T06:15:41Z

System.Speech.Tests test failure seems unrelated.

wfurt · 2021-01-12T07:00:02Z

/backport to release/5.0

github-actions · 2021-01-12T07:01:02Z

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/479577114

github-actions · 2021-01-12T07:03:15Z

@wfurt backporting to release/5.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: fallback to GetAddrInfoW if GetAddrInfoExW fails
error: sha1 information is lacking or useless (src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 fallback to GetAddrInfoW if GetAddrInfoExW fails
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

fallback to GetAddrInfoW if GetAddrInfoExW fails

a319e12

Dotnet-GitSync-Bot added the area-System.Net label Dec 9, 2020

CarnaViire reviewed Dec 9, 2020

View reviewed changes

Comment thread src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs Outdated

runfoapp Bot mentioned this pull request Dec 9, 2020

Inability to unzip assets during build on Unix x64 #32805

Closed

scalablecory reviewed Dec 9, 2020

View reviewed changes

Comment thread src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs Outdated

wfurt self-assigned this Dec 9, 2020

Update src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs

dc31a54

Co-authored-by: Cory Nelson <phrosty@gmail.com>

danmoseley reviewed Dec 9, 2020

View reviewed changes

Comment thread src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs Outdated

jkotas reviewed Dec 10, 2020

View reviewed changes

Comment thread src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs Outdated

Update src/libraries/System.Net.NameResolution/src/System/Net/NameRes…

13dc1f8

…olutionPal.Windows.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>

stephentoub reviewed Dec 11, 2020

View reviewed changes

Comment thread src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

stephentoub reviewed Dec 11, 2020

View reviewed changes

Comment thread src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

feedback from review

2c1a5a3

update fakes

559ce36

MihaZupan reviewed Dec 15, 2020

View reviewed changes

danmoseley reviewed Dec 18, 2020

View reviewed changes

Comment thread src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs Outdated

wfurt added 5 commits January 8, 2021 01:21

attempt to fix telemetry

9338248

Merge branch 'dns_29935' of https://github.com/wfurt/runtime into dns…

f46e781

…_29935

remofe forced telemetry

5444a57

fix spelling

5415735

improve duration precision

98e17c1

stephentoub reviewed Jan 8, 2021

View reviewed changes

wfurt added 2 commits January 10, 2021 16:17

task refactor

49e0ced

update fakes again

5c4dce1

stephentoub reviewed Jan 11, 2021

View reviewed changes

Comment thread src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs Outdated

stephentoub approved these changes Jan 11, 2021

View reviewed changes

rename function

ac49cb8

wfurt merged commit 1655959 into dotnet:master Jan 12, 2021

wfurt mentioned this pull request Jan 13, 2021

[release/5.0] make async name resolution working from impersonificated context #46897

Merged

wfurt mentioned this pull request Jan 25, 2021

Support Async DNS lookup on older Windows from impersonificated context #47435

Merged

karelz added this to the 6.0.0 milestone Jan 26, 2021

karelz mentioned this pull request Feb 2, 2021

Dns.GetHostAddressesAsync fails with SocketException when called during impersonation #29935

Closed

ghost locked as resolved and limited conversation to collaborators Feb 25, 2021

wfurt deleted the dns_29935 branch January 7, 2026 20:00

	// If it fails, we will fall-back to ThreadPool as well.
	// If it fails, we will fall back to ThreadPool as well.

Conversation

wfurt commented Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Dec 9, 2020

Uh oh!

Uh oh!

CarnaViire commented Dec 9, 2020

Uh oh!

scalablecory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wfurt commented Dec 9, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wfurt commented Dec 11, 2020

Uh oh!

azure-pipelines Bot commented Dec 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wfurt commented Jan 12, 2021

Uh oh!

wfurt commented Jan 12, 2021

Uh oh!

github-actions Bot commented Jan 12, 2021

Uh oh!

github-actions Bot commented Jan 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

wfurt commented Dec 9, 2020 •

edited

Loading