[WASI] System.Net.NameResolution #107351

pavelsavara · 2024-09-04T14:06:01Z

based on unix
use #ifdef in pal_networking.c
drop pal_networking_wasi.c
WASI doesn't have getnameinfo -> PlatformNotSupportedException
WASI doesn't have gethostname, we return localhost
WASI is single threaded
- dummy implementation of NameResolutionTelemetry
- no suport for Dns.Begin*, Dns.End APIs -> PlatformNotSupportedException
[UnsupportedOSPlatform] will come bit later
conversion for EAI_SYSTEM code
addded System.Net.NameResolution.Functional.Tests to WASI smoke test on CI

Created ActiveIssue #107339

dotnet-policy-service · 2024-09-04T14:06:47Z

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

ilonatommy

Looks good. There's a small nit, sometimes you do #endif // !TARGET_WASI and sometimes #endif // TARGET_WASI for if !defined(TARGET_WASI) in C code, it's not uniform.

src/native/libs/System.Native/pal_networking.c

lewing

For in-box assemblies use OperatingSystem.IsWasi() instead of #ifdefs whenever possible, the branch will be optimized out in nearly every case. In the pal try to use feature specific #ifdefs like HAVE_EPOLL etc.

pavelsavara · 2024-09-11T10:04:08Z

pavelsavara · 2024-09-11T13:19:28Z

For in-box assemblies use OperatingSystem.IsWasi() instead of #ifdefs whenever possible, the branch will be optimized out in nearly every case. In the pal try to use feature specific #ifdefs like HAVE_EPOLL etc.

Done

src/native/libs/configure.cmake

wfurt

LGTM. Seems like this is mostly plumbing to get it build, right?

src/native/libs/configure.cmake

...ibraries/System.Net.NameResolution/tests/PalTests/System.Net.NameResolution.Pal.Tests.csproj

src/libraries/System.Net.NameResolution/tests/FunctionalTests/TelemetryTest.cs

...m.Net.NameResolution/tests/FunctionalTests/System.Net.NameResolution.Functional.Tests.csproj

src/libraries/System.Net.NameResolution/tests/FunctionalTests/MetricsTest.cs

src/libraries/System.Net.NameResolution/tests/FunctionalTests/LoggingTest.cs

src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostEntryTest.cs

src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionTelemetry.cs

src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs

Co-authored-by: Alexander Köplinger <[email protected]>

…LoggingTest.cs Co-authored-by: Alexander Köplinger <[email protected]>

…TelemetryTest.cs Co-authored-by: Alexander Köplinger <[email protected]>

…MetricsTest.cs Co-authored-by: Alexander Köplinger <[email protected]>

…olutionPal.Unix.cs Co-authored-by: Alexander Köplinger <[email protected]>

akoeplinger · 2024-09-12T12:58:46Z

src/native/libs/System.Native/pal_networking.c

    *pathOffset = offsetof(struct sockaddr_un, sun_path);
    *pathSize = sizeof(domainSocket.sun_path);
+#else // HAVE_SOCKADDR_UN_SUN_PATH
+    *pathOffset = 0;
+    *pathSize = 0;


should this return -1 like in the previous WASI implementation?

looking at the managed code we have Debug.Assert that checks pathSize >= 92 in UnixDomainSocketEndPoint.Unix.cs, we should probably return -1 here again and add code to not call GetDomainSocketSizes on Wasi there.

That's in static constructor and would throw.
Let's revisit that on the socket PR #106977 I added todo there

akoeplinger · 2024-09-12T13:06:39Z

src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetHostName.cs

+            if (OperatingSystem.IsWasi())
+            {
+                return "localhost";
+            }


nit: it feels slightly wrong doing this special case in the Interop.GetHostName.cs managed binding layer, but calling into native code just for getting a constant string feels wrong too.

Btw. after looking at it some more I think we have an issue on Browser: we do use emscripten's gethostname() which returns the string emscripten and we just paper over it here:

runtime/src/libraries/System.Private.CoreLib/src/System/Environment.Browser.cs

Lines 13 to 15 in 4cdbfdc

// In the mono runtime, this maps to gethostname, which returns 'emscripten'.

// Returning the value here allows us to exclude more of the runtime.

public static string MachineName => "localhost";

But Interop.Sys.GetHostName() is used in more places and those would get the "wrong" hostname on Browser. I think I'd be in favor of unifying this and adding the special case for Browser here too.

Let's do separate PR for that

Co-authored-by: Alexander Köplinger <[email protected]>

pavelsavara added arch-wasm WebAssembly architecture area-System.Net os-wasi Related to WASI variant of arch-wasm labels Sep 4, 2024

pavelsavara added this to the 10.0.0 milestone Sep 4, 2024

pavelsavara self-assigned this Sep 4, 2024

rebase

90a7b23

pavelsavara force-pushed the wasi_dns branch from dc15610 to 90a7b23 Compare September 6, 2024 15:13

pavelsavara requested review from lewing and maraf September 6, 2024 15:22

pavelsavara marked this pull request as ready for review September 6, 2024 15:23

pavelsavara requested review from karelz and wfurt September 6, 2024 15:23

pavelsavara added 2 commits September 9, 2024 10:14

fix build

a473a2a

fix build

68c4265

This comment was marked as resolved.

Sign in to view

fix linux

787a123

ilonatommy approved these changes Sep 9, 2024

View reviewed changes

src/native/libs/System.Native/pal_networking.c Show resolved Hide resolved

lewing reviewed Sep 10, 2024

View reviewed changes

pavelsavara added 2 commits September 11, 2024 15:08

feedback

3bbccad

more

c6dc3da

pavelsavara requested review from lewing and ilonatommy September 11, 2024 13:19

pavelsavara added 4 commits September 11, 2024 16:13

fix

cb122e8

fix

43360f9

fix

f8ba745

fix

c414ac4

build-analysis bot mentioned this pull request Sep 11, 2024

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Open

3 tasks

fix

7e40a3d

pavelsavara commented Sep 11, 2024

View reviewed changes

src/native/libs/configure.cmake Outdated Show resolved Hide resolved

fix?

69421fb

wfurt approved these changes Sep 11, 2024

View reviewed changes

akoeplinger reviewed Sep 12, 2024

View reviewed changes

pavelsavara and others added 7 commits September 12, 2024 10:55

Update src/native/libs/configure.cmake

9aea115

Co-authored-by: Alexander Köplinger <[email protected]>

Update src/libraries/System.Net.NameResolution/tests/FunctionalTests/…

195a475

…LoggingTest.cs Co-authored-by: Alexander Köplinger <[email protected]>

Update src/libraries/System.Net.NameResolution/tests/FunctionalTests/…

aac1d37

…TelemetryTest.cs Co-authored-by: Alexander Köplinger <[email protected]>

Update src/libraries/System.Net.NameResolution/tests/FunctionalTests/…

806179d

…MetricsTest.cs Co-authored-by: Alexander Köplinger <[email protected]>

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

4da7bc9

…olutionPal.Unix.cs Co-authored-by: Alexander Köplinger <[email protected]>

feedback

145ca0c

feedback

240f9e8

akoeplinger approved these changes Sep 12, 2024

View reviewed changes

This was referenced Sep 12, 2024

[WASI] sockets #106977

Draft

[wasm] unify Interop.GetHostName #107741

Open

pavelsavara merged commit a833cfb into dotnet:main Sep 12, 2024
158 of 161 checks passed

pavelsavara deleted the wasi_dns branch September 12, 2024 14:41

build-analysis bot mentioned this pull request Sep 12, 2024

System.Data.OleDb.Tests timeout in net48 x86 Release leg #87783

Open

jtschuster pushed a commit to jtschuster/runtime that referenced this pull request Sep 17, 2024

[WASI] System.Net.NameResolution (dotnet#107351)

cff13cc

Co-authored-by: Alexander Köplinger <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WASI] System.Net.NameResolution #107351

[WASI] System.Net.NameResolution #107351

pavelsavara commented Sep 4, 2024 •

edited

Loading

dotnet-policy-service bot commented Sep 4, 2024

This comment was marked as resolved.

ilonatommy left a comment

lewing left a comment

pavelsavara commented Sep 11, 2024

pavelsavara commented Sep 11, 2024

wfurt left a comment

akoeplinger Sep 12, 2024

pavelsavara Sep 12, 2024

akoeplinger Sep 12, 2024

pavelsavara Sep 12, 2024

pavelsavara Sep 12, 2024

	// In the mono runtime, this maps to gethostname, which returns 'emscripten'.
	// Returning the value here allows us to exclude more of the runtime.
	public static string MachineName => "localhost";

[WASI] System.Net.NameResolution #107351

[WASI] System.Net.NameResolution #107351

Conversation

pavelsavara commented Sep 4, 2024 • edited Loading

dotnet-policy-service bot commented Sep 4, 2024

This comment was marked as resolved.

ilonatommy left a comment

Choose a reason for hiding this comment

lewing left a comment

Choose a reason for hiding this comment

pavelsavara commented Sep 11, 2024

pavelsavara commented Sep 11, 2024

wfurt left a comment

Choose a reason for hiding this comment

akoeplinger Sep 12, 2024

Choose a reason for hiding this comment

pavelsavara Sep 12, 2024

Choose a reason for hiding this comment

akoeplinger Sep 12, 2024

Choose a reason for hiding this comment

pavelsavara Sep 12, 2024

Choose a reason for hiding this comment

pavelsavara Sep 12, 2024

Choose a reason for hiding this comment

pavelsavara commented Sep 4, 2024 •

edited

Loading