Skip to content
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

NameResolutionPal.Unix: fixed loss of address family information in async lookup #47171

Merged
merged 2 commits into from
Jan 22, 2021

Conversation

gfoidl
Copy link
Member

@gfoidl gfoidl commented Jan 19, 2021

Fixes #47103

C's designated struct initializer worked at the time it was suggested as for ar_request a static const value was used.
Then address family handling got added to master and while resolving the merge conflict and refactoring a bit of code the static const value was gone, and while (recursively) initializing the struct state the hint-field got lost (overwritten)*, and so the address family information that is used in the (native) callback of the async lookup.

This PR changes the initialization of the state-struct, so that the information isn't lost.

I've also removed some dead code, that was an artefact of refactoring and it's removal got lost during all the changes in #34633.

I'm wondering why the outerloop-test in my linux-vm didn't fail back then...

* quoting the docs:

Omitted fields are implicitly initialized the same as for objects that have static storage duration.

@ghost
Copy link

ghost commented Jan 19, 2021

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

Issue Details

Fixes #47103

C's designated struct initializer worked at the time it was suggested as for ar_request a static const value was used.
Then address family handling got added to master and while resolving the merge conflict and refactoring a bit of code the static const value was gone, and while (recursively) initializing the struct state the hint-field got lost (overwritten)*, and so the address family information that is used in the (native) callback of the async lookup.

This PR changes the initialization of the state-struct, so that the information isn't lost.

I've also removed some dead code, that was an artefact of refactoring and it's removal got lost during all the changes in #34633.

I'm wondering why the outerloop-test in my linux-vm didn't fail back then...

* quoting the docs:

Omitted fields are implicitly initialized the same as for objects that have static storage duration.

Author: gfoidl
Assignees: -
Labels:

area-System.Net

Milestone: -

@wfurt
Copy link
Member

wfurt commented Jan 21, 2021

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

if (!TryConvertAddressFamilyPalToPlatform(addressFamily, &platformFamily))
{
return GetAddrInfoErrorFlags_EAI_FAMILY;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What makes this dead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like the TrySetAddressFamily() bellow does it inside so it seems like duplication to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, not that it's dead code but that it's unnecessary because the same check is performed immediately after. Ok.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now

if (!TrySetAddressFamily(addressFamily, &state->hint))
sets the address family (after the code for sync and async got unified).

The removed code is a left-over from before that unification. The platformFamily isn't used anymore, so it's "dead".
This was forgotten in the other PR (due the work in single file host build).

(For completeness...late reply due being in different timezone 😉).

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
It seems like all the outerloop failures are not related e.g. I did no see any name resolution related failure.

@wfurt wfurt merged commit 2f25931 into dotnet:master Jan 22, 2021
@gfoidl gfoidl deleted the nameresolution-unix-bug-fix branch January 23, 2021 11:34
@karelz karelz added this to the 6.0.0 milestone Jan 26, 2021
gfoidl added a commit to gfoidl/dotnet-runtime that referenced this pull request Feb 23, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test failed: System.Net.NameResolution.Tests.GetHostEntryTest.DnsGetHostEntry_LocalHost_AddressFamilySpecific
5 participants