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 enabled async name resolution #34633

Merged
merged 37 commits into from
Jan 14, 2021
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
fc09ca1
Native implementation
gfoidl Apr 6, 2020
8c955bc
Native tests
gfoidl Apr 6, 2020
cf13c70
Managed implementation
gfoidl Apr 6, 2020
4614267
Managed tests
gfoidl Apr 6, 2020
1b2daab
Revert Interop.GetHostName change -- it's needed at other places too
gfoidl Apr 7, 2020
b9629e9
Fixed builds failures due to "unused parameter"
gfoidl Apr 7, 2020
6f64c86
Native: move struct addrinfo hint from stack-alloc to heap-alloc
gfoidl Apr 7, 2020
e7df0cc
PR feedback
gfoidl Apr 8, 2020
d7cb2be
Fixed leak in tests.c according to valgrind's run
gfoidl Apr 9, 2020
74b3031
Fixed bug due to marshalled string argument
gfoidl Apr 9, 2020
775ff72
More managed PalTests
gfoidl Apr 9, 2020
ec8522f
Revert "Native tests"
gfoidl Apr 9, 2020
298cf03
Handle the case of HostName="" and tests for this
gfoidl Apr 9, 2020
458579b
Use flexible array member to hold the address in struct state
gfoidl Apr 9, 2020
8f69164
Nits in native layer
gfoidl Apr 10, 2020
8cec5fb
Merge branch 'master' into unix-async-name-resolution
gfoidl Jul 1, 2020
f3ebd6a
Merge branch 'master' into unix-async-name-resolution
gfoidl Aug 13, 2020
cf45cc9
Merge branch 'master' into unix-async-name-resolution
gfoidl Dec 15, 2020
43f889f
Fixed native merge conflict + added AddressFamily-handling
gfoidl Dec 15, 2020
bd16c74
Fixed managed merge conflicts + added AddressFamily-handling
gfoidl Dec 15, 2020
ed5c39f
Updated NameResolutionPalTests for AddressFamily
gfoidl Dec 15, 2020
b8efde5
Removed EnsureSocketsAreInitialized
gfoidl Dec 15, 2020
04dbd8e
Fixed bug at native side
gfoidl Dec 15, 2020
37e4dd3
Refactor setting of the address family into TrySetAddressFamily
gfoidl Dec 15, 2020
7176378
Fixed unused parameter warning / failure
gfoidl Dec 15, 2020
b64867b
Little cleanup
gfoidl Dec 16, 2020
900834e
Fixed (unrelated) test failures
gfoidl Dec 16, 2020
198717e
Made LoggingTest async instead of GetAwaiter().GetResult()
gfoidl Dec 16, 2020
f85c316
Use function pointer
gfoidl Dec 16, 2020
438c75a
Use OperatinngSystem instead of RuntimeInformation in tests
gfoidl Dec 18, 2020
89c266f
Merge branch 'master' into unix-async-name-resolution
gfoidl Dec 18, 2020
33da8da
PR Feedback for CMake
gfoidl Dec 18, 2020
fa9c6f9
Update src/libraries/Native/Unix/System.Native/extra_libs.cmake
VSadov Dec 18, 2020
8266dd3
Revert "Update src/libraries/Native/Unix/System.Native/extra_libs.cmake"
gfoidl Jan 8, 2021
75e52c7
Another to build single file host
gfoidl Jan 8, 2021
89a7c5a
Merge branch 'master' into unix-async-name-resolution
gfoidl Jan 14, 2021
dd1e245
Test for !Windows instead excluding several Unix-flavors
gfoidl Jan 14, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Runtime.InteropServices;

internal static partial class Interop
Expand Down Expand Up @@ -33,9 +32,17 @@ internal unsafe struct HostEntry
internal int IPAddressCount; // Number of IP addresses in the list
}

internal unsafe delegate void GetHostEntryForNameCallback(HostEntry* entry, int status);

[DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_PlatformSupportsGetAddrInfoAsync")]
internal static extern bool PlatformSupportsGetAddrInfoAsync();

[DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetHostEntryForName")]
internal static extern unsafe int GetHostEntryForName(string address, HostEntry* entry);

[DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetHostEntryForNameAsync")]
internal static extern unsafe int GetHostEntryForNameAsync(string address, HostEntry* entry, GetHostEntryForNameCallback callback);

[DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_FreeHostEntry")]
internal static extern unsafe void FreeHostEntry(HostEntry* entry);
}
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/Native/Unix/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -210,3 +210,5 @@ endif()
if(CLR_CMAKE_TARGET_OSX OR CLR_CMAKE_TARGET_IOS)
add_subdirectory(System.Security.Cryptography.Native.Apple)
endif()

enable_testing()
1 change: 1 addition & 0 deletions src/libraries/Native/Unix/Common/pal_config.h.in
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#cmakedefine01 HAVE_F_DUPFD_CLOEXEC
#cmakedefine01 HAVE_O_CLOEXEC
#cmakedefine01 HAVE_GETIFADDRS
#cmakedefine01 HAVE_GETADDRINFO_A
#cmakedefine01 HAVE_UTSNAME_DOMAINNAME
#cmakedefine01 HAVE_STAT64
#cmakedefine01 HAVE_VFORK
Expand Down
35 changes: 35 additions & 0 deletions src/libraries/Native/Unix/System.Native/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ if (GEN_SHARED_LIB)
endif ()
endif ()
install_with_stripped_symbols (System.Native PROGRAMS .)

if (HAVE_GETADDRINFO_A)
target_link_libraries(System.Native anl)
endif ()
endif ()

add_library(System.Native-Static
Expand All @@ -76,3 +80,34 @@ endif ()
set_target_properties(System.Native-Static PROPERTIES OUTPUT_NAME System.Native CLEAN_DIRECT_OUTPUT 1)

install (TARGETS System.Native-Static DESTINATION .)

if (HAVE_GETADDRINFO_A)
add_executable(tests
tests.c
)

target_link_libraries(tests
System.Native
pthread
)

enable_testing()

add_test(no_args tests)
set_tests_properties(no_args PROPERTIES WILL_FAIL TRUE)
set_tests_properties(no_args PROPERTIES TIMEOUT 1)

add_test(hostname_local tests localhost)
set_tests_properties(hostname_local PROPERTIES TIMEOUT 1)

add_test(hostname_ok tests google.at)
set_tests_properties(hostname_ok PROPERTIES TIMEOUT 1)

add_test(hostname_invalid tests abc.test01)
set_tests_properties(hostname_invalid PROPERTIES WILL_FAIL TRUE)
set_tests_properties(hostname_invalid PROPERTIES TIMEOUT 1)

add_test(hostname_null tests null)
set_tests_properties(hostname_null PROPERTIES WILL_FAIL TRUE)
set_tests_properties(hostname_null PROPERTIES TIMEOUT 1)
endif ()
175 changes: 148 additions & 27 deletions src/libraries/Native/Unix/System.Native/pal_networking.c
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@
#if HAVE_LINUX_CAN_H
#include <linux/can.h>
#endif
#if HAVE_GETADDRINFO_A
#include <stdatomic.h>
#include <signal.h>
#endif
#if HAVE_KQUEUE
#if KEVENT_HAS_VOID_UDATA
static void* GetKeventUdata(uintptr_t udata)
Expand Down Expand Up @@ -253,32 +257,14 @@ static int32_t CopySockAddrToIPAddress(sockaddr* addr, sa_family_t family, IPAdd
return -1;
}

int32_t SystemNative_GetHostEntryForName(const uint8_t* address, HostEntry* entry)
static int32_t GetHostEntries(const uint8_t* address, struct addrinfo* info, HostEntry* entry)
{
if (address == NULL || entry == NULL)
{
return GetAddrInfoErrorFlags_EAI_BADARG;
}

int32_t ret = GetAddrInfoErrorFlags_EAI_SUCCESS;

struct addrinfo* info = NULL;
#if HAVE_GETIFADDRS
struct ifaddrs* addrs = NULL;
#endif

// Get all address families and the canonical name
struct addrinfo hint;
memset(&hint, 0, sizeof(struct addrinfo));
hint.ai_family = AF_UNSPEC;
hint.ai_flags = AI_CANONNAME;

int result = getaddrinfo((const char*)address, NULL, &hint, &info);
if (result != 0)
{
return ConvertGetAddrInfoAndGetNameInfoErrorsToPal(result);
}

entry->CanonicalName = NULL;
entry->Aliases = NULL;
entry->IPAddressList = NULL;
Expand Down Expand Up @@ -306,7 +292,8 @@ int32_t SystemNative_GetHostEntryForName(const uint8_t* address, HostEntry* entr

#if HAVE_GETIFADDRS
char name[_POSIX_HOST_NAME_MAX];
result = gethostname((char*)name, _POSIX_HOST_NAME_MAX);

int result = gethostname((char*)name, _POSIX_HOST_NAME_MAX);

bool includeIPv4Loopback = true;
bool includeIPv6Loopback = true;
Expand Down Expand Up @@ -356,6 +343,9 @@ int32_t SystemNative_GetHostEntryForName(const uint8_t* address, HostEntry* entr
}
}
}
#else
// Actually not needed, but to use the parameter in case of HAVE_GETIFADDRS expands to 0.
address = NULL;
#endif

if (entry->IPAddressCount > 0)
Expand Down Expand Up @@ -432,6 +422,137 @@ int32_t SystemNative_GetHostEntryForName(const uint8_t* address, HostEntry* entr
return ret;
}

#if HAVE_GETADDRINFO_A
struct GetAddrInfoAsyncState
{
struct gaicb gai_request;
struct gaicb* gai_requests;
struct sigevent sigevent;

const uint8_t* address;
HostEntry* entry;
GetHostEntryForNameCallback callback;
};

static void GetHostEntryForNameAsyncComplete(sigval_t context)
{
struct GetAddrInfoAsyncState* state = (struct GetAddrInfoAsyncState*)context.sival_ptr;

atomic_thread_fence(memory_order_acquire);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the fence necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

This callback comes in on another thread. It ensures visibility to this thread of everything in state.

getaddrinfo_a probably does something like this already, so it might be safe to remove, but it isn't documented.


GetHostEntryForNameCallback callback = state->callback;

int result = gai_error(&state->gai_request);
HostEntry* entry = state->entry;

int ret = ConvertGetAddrInfoAndGetNameInfoErrorsToPal(result);

if (result == 0)
{
const uint8_t* address = state->address;
struct addrinfo* info = state->gai_request.ar_result;

ret = GetHostEntries(address, info, entry);
}

free(state);

if (callback != NULL)
{
callback(entry, ret);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
int result = gai_error(&state->gai_request);
HostEntry* entry = state->entry;
int ret = ConvertGetAddrInfoAndGetNameInfoErrorsToPal(result);
if (result == 0)
{
const uint8_t* address = state->address;
struct addrinfo* info = state->gai_request.ar_result;
ret = GetHostEntries(address, info, entry);
}
free(state);
if (callback != NULL)
{
callback(entry, ret);
}
int ret = ConvertGetAddrInfoAndGetNameInfoErrorsToPal(gai_error(&state->gai_request));
if (ret == 0)
{
const uint8_t* address = state->address;
struct addrinfo* info = state->gai_request.ar_result;
ret = GetHostEntries(address, info, entry);
}
if (callback != NULL)
{
callback(state->entry, ret);
}
free(state);

Copy link
Member Author

Choose a reason for hiding this comment

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

I had the free(state) intentionally before calling the callback.

  • entry is passed from managed to native and via callback back to managed -- so the position of free(state) doesn't change anything
  • if the callback (managed side) has an exception state won't be freed an so this can be a memory leak -- although ProcessResult on the managed side (the "callback worker") has a try-finally

I prefer leaving the free(state) before the callback. When there is a good reason to move it after the callback, I'll do it for sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Both are equivalent, yes, but the suggested change is slightly shorter (and doesn't have a variable that's attributed to but never read from, which is cleaner).

Copy link
Member Author

Choose a reason for hiding this comment

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

OK.

Copy link
Contributor

Choose a reason for hiding this comment

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

(It's just a suggestion, no need to change it at all.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but I like it as the code looks cleaner.
Anyway I highly appreciate the feedback here! (My C is a bit rusty, so it's good to get some hints.)

}
#endif

static void GetHostEntryForNameCreateHint(struct addrinfo* hint)
{
// Get all address families and the canonical name

memset(hint, 0, sizeof(struct addrinfo));
hint->ai_family = AF_UNSPEC;
hint->ai_flags = AI_CANONNAME;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the parameter to getaddrinfo() is constant, this could be defined statically and the address passed like that everywhere where it's needed:

Suggested change
static void GetHostEntryForNameCreateHint(struct addrinfo* hint)
{
// Get all address families and the canonical name
memset(hint, 0, sizeof(struct addrinfo));
hint->ai_family = AF_UNSPEC;
hint->ai_flags = AI_CANONNAME;
}
static const struct addrinfo s_hostEntryForNameCreateHint = { .ai_family = AI_UNSPEC, .ai_flags = AI_CANONNAME };


int32_t SystemNative_PlatformSupportsGetAddrInfoAsync()
{
#if HAVE_GETADDRINFO_A
return 1;
#else
return 0;
#endif
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to avoid this P/Invoke every time?

Also, since cmakedefine01 is used, maybe this could be rewritten as:

Suggested change
int32_t SystemNative_PlatformSupportsGetAddrInfoAsync()
{
#if HAVE_GETADDRINFO_A
return 1;
#else
return 0;
#endif
}
int32_t SystemNative_PlatformSupportsGetAddrInfoAsync()
{
return HAVE_GETADDRINFO_A;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

It's cached on the managed side.

Nice trick to return the define. I didn't think about that. Thx.

Will address the other points tomorrow.


int32_t SystemNative_GetHostEntryForName(const uint8_t* address, HostEntry* entry)
{
if (address == NULL || entry == NULL)
{
return GetAddrInfoErrorFlags_EAI_BADARG;
}

struct addrinfo hint;
GetHostEntryForNameCreateHint(&hint);

struct addrinfo* info = NULL;

int result = getaddrinfo((const char*)address, NULL, &hint, &info);
if (result != 0)
{
return ConvertGetAddrInfoAndGetNameInfoErrorsToPal(result);
}

return GetHostEntries(address, info, entry);
}

int32_t SystemNative_GetHostEntryForNameAsync(const uint8_t* address, HostEntry* entry, GetHostEntryForNameCallback callback)
{
#if HAVE_GETADDRINFO_A
if (address == NULL)
{
return GetAddrInfoErrorFlags_EAI_BADARG;
}

struct addrinfo hint;
GetHostEntryForNameCreateHint(&hint);

struct GetAddrInfoAsyncState* state = malloc(sizeof(struct GetAddrInfoAsyncState));

state->gai_request.ar_name = (const char*)address;
state->gai_request.ar_service = NULL;
state->gai_request.ar_request = &hint;
state->gai_request.ar_result = NULL;
state->gai_requests = &state->gai_request;

state->sigevent.sigev_notify = SIGEV_THREAD;
state->sigevent.sigev_value.sival_ptr = state;
state->sigevent.sigev_notify_function = GetHostEntryForNameAsyncComplete;

state->address = address;
state->entry = entry;
state->callback = callback;
Copy link
Contributor

@lpereira lpereira Apr 7, 2020

Choose a reason for hiding this comment

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

Suggested change
struct GetAddrInfoAsyncState* state = malloc(sizeof(struct GetAddrInfoAsyncState));
state->gai_request.ar_name = (const char*)address;
state->gai_request.ar_service = NULL;
state->gai_request.ar_request = &hint;
state->gai_request.ar_result = NULL;
state->gai_requests = &state->gai_request;
state->sigevent.sigev_notify = SIGEV_THREAD;
state->sigevent.sigev_value.sival_ptr = state;
state->sigevent.sigev_notify_function = GetHostEntryForNameAsyncComplete;
state->address = address;
state->entry = entry;
state->callback = callback;
struct GetAddrInfoAsyncState* state = malloc(sizeof(*state));
if (state == NULL) return oops_no_memory;
*state = (struct GetAddrInfoAsyncState) {
.gai_request = {
.ar_name = ...,
.ar_service = ...,
...,
},
.sigevent = {
.sigev_notify = ...,
...,
},
.address = ...,
...
};


atomic_thread_fence(memory_order_release);

int32_t result = getaddrinfo_a(GAI_NOWAIT, &state->gai_requests, 1, &state->sigevent);

if (result != 0)
{
free(state);
return ConvertGetAddrInfoAndGetNameInfoErrorsToPal(result);
}

return result;
#else
// Actually not needed, but otherwise the parameters are unused.
if (address != NULL && entry != NULL && callback != NULL)
{
return -1;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Actually not needed, but otherwise the parameters are unused.
if (address != NULL && entry != NULL && callback != NULL)
{
return -1;
}
(void)address;
(void)entry;
(void)calback;


// GetHostEntryForNameAsync is not supported on this platform.
return -1;
#endif
}

void SystemNative_FreeHostEntry(HostEntry* entry)
{
if (entry != NULL)
Expand Down Expand Up @@ -468,13 +589,13 @@ static inline NativeFlagsType ConvertGetNameInfoFlagsToNative(int32_t flags)
}

int32_t SystemNative_GetNameInfo(const uint8_t* address,
int32_t addressLength,
int8_t isIPv6,
uint8_t* host,
int32_t hostLength,
uint8_t* service,
int32_t serviceLength,
int32_t flags)
int32_t addressLength,
int8_t isIPv6,
uint8_t* host,
int32_t hostLength,
uint8_t* service,
int32_t serviceLength,
int32_t flags)
{
assert(address != NULL);
assert(addressLength > 0);
Expand Down
6 changes: 5 additions & 1 deletion src/libraries/Native/Unix/System.Native/pal_networking.h
Original file line number Diff line number Diff line change
Expand Up @@ -310,10 +310,14 @@ typedef struct
uint32_t Padding; // Pad out to 8-byte alignment
} SocketEvent;

PALEXPORT int32_t SystemNative_PlatformSupportsGetAddrInfoAsync(void);

PALEXPORT int32_t SystemNative_GetHostEntryForName(const uint8_t* address, HostEntry* entry);

PALEXPORT void SystemNative_FreeHostEntry(HostEntry* entry);
typedef void (*GetHostEntryForNameCallback)(HostEntry* entry, int status);
PALEXPORT int32_t SystemNative_GetHostEntryForNameAsync(const uint8_t* address, HostEntry* entry, GetHostEntryForNameCallback callback);

PALEXPORT void SystemNative_FreeHostEntry(HostEntry* entry);

PALEXPORT int32_t SystemNative_GetNameInfo(const uint8_t* address,
int32_t addressLength,
Expand Down
Loading