-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[WASI] sockets #106977
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
[WASI] sockets #106977
Conversation
|
Tagging subscribers to this area: @dotnet/ncl |
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEngine.Wasi.cs
Outdated
Show resolved
Hide resolved
f8c2ff3 to
3ba86b1
Compare
3ba86b1 to
cc4e041
Compare
cc4e041 to
db54296
Compare
db54296 to
02bf1d9
Compare
42281bc to
c7280f7
Compare
c7280f7 to
0bf688e
Compare
f9fb647 to
9f7b3d9
Compare
dicej
left a comment
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.
LGTM. Thanks for doing all this!
I think we should eventually consider supporting blocking operations even without multithreading. Some single-threaded command line apps are content to use blocking operations since they have nothing else to do besides wait for the network. And although it's clearly an antipattern, some popular libraries (e.g. Microsoft.Data.SqlClient) do sync-on-async operations. Clearly we want to fix those libraries anyway (and the SqlClient folks are already planning to do so), but in the meantime I'd be inclined to maximize compatibility by supporting the existing patterns.
Anyway, I'm fine with the PNEs for blocking ops in this PR -- it's a reasonable default.
I was thinking about it. Filesystem and DNS requests are blocking after all.
|
Not necessarily -- WASI provides pollables for DNS and (most?) filesystem ops. Granted, the host implementation may require blocking + multithreading due to OS limitations, but from the guest's perspective they're async.
Timeouts could still work; the blocking API would still use pollables in the implementation.
That's fair; I'm new enough to .NET that I don't know how tightly bound the concepts of blocking I/O and multithreading are. My only real-world data point so far is SqlClient, which worked fine when we added blocking I/O to the prototype.
We can call Your other points seem valid, though. |
But we consume that via
We would learn that something got unblocked, but we can't call the continuations from inside blocking call. |
👍
There wouldn't be any |
We are getting very off-topic on this PR. But I think that it would go |
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.
A question that came up last week during the BCA plummers summit was if this will automatically pull in the wasi-socket imports for all wasm components after implemented or only when System.net.Sockets api's are used?
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/src/System/Net/Sockets/TCPClient.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated
Show resolved
Hide resolved
| { | ||
| if (optionValue == 0) | ||
| { | ||
| UpdateStatusAfterSocketOptionErrorAndThrowException(SocketError.ProtocolOption); |
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.
I'm not sure we need to throw if the platform does more than asked for. If we have concerns we can can strip to info internally IMHO.
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 is only throwing when user asks to disable PacketInformation
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.
right. We can still simply throw it out. And not receiving the info is default AFAIK. mostly perf improvement IMHo but that probably does not matter for your use case.
| { | ||
| internal sealed unsafe class SocketAsyncEngine | ||
| { | ||
| internal static readonly bool InlineSocketCompletionsEnabled = Environment.GetEnvironmentVariable("DOTNET_SYSTEM_NET_SOCKETS_INLINE_COMPLETIONS") == "1"; |
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.
I don;t think you need this. This is related to thread pool optimization and there are no threads on WASI, right?
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.
There is InlineSocketCompletionsEnabled -> SafeSocketHandle.PreferInlineCompletions -> context.PreferInlineCompletions which needs it.
I don't understand why this is not a static member on SafeSocketHandle. Do we expect the env variable to change mid-flight ?
For WASI I made it constant on SocketAsyncEngine
wfurt
left a comment
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.
generally looks ok to me. There many check IsWasi. It may be worth to come up with variable describing the limitation - ... like SupportsSync ... but I guess that is just style and personal preference.
When the WASI public surface stabilizes bit more, we will add But for now, you are right that expressing reason is better. I added |
It will create that dependency, I think via |
|
Well, I had couple of questions but already got answer to them from the discussions/messages under this PR, everything looks good now to me. |
Limitations
permanent design of WASI ?
SO_LINGER->PlatformNotSupportedExceptionDONTFRAGMENT->IP_MTU_DISCOVER->PlatformNotSupportedExceptionPlatformNotSupportedExceptionPlatformNotSupportedExceptionDualModeIPv6+IPv4PlatformNotSupportedExceptionSocketconstructor withoutaddressFamilyparameter uses IPv4 only!PlatformNotSupportedExceptionSO_REUSEADDRtemporary limitation
IOControl->PlatformNotSupportedExceptionuntilSO_RCVTIMEOandSO_SNDTIMEO->PlatformNotSupportedExceptionfor nowuntil multi-threading WASI
PlatformNotSupportedExceptionConnect,Send,Receive,Poll,Accept,SendFile,SendTo,ReceiveFromSelect,PollPlatformNotSupportedException. Because the *End methods are blocking.SOCK_NONBLOCKon libc levelSocket.Blockingis defaultfalseas opposed to all other dotnet platforms.NetworkStreamare also ->PlatformNotSupportedExceptionChanges in this PR
sendtoandrecvfromfor WASI instead of missingrecvmsgandsendmsg.SystemNative_GetWasiSocketDescriptor->wasi-libcinternaldescriptor_table_get_refWasiEventLoop.RegisterWasiPollHookfor the same reasonSafeSocketHandle.Unix.cs,Socket.Unix.cs,SocketAsyncContext.Unix.csSocketAsyncEngine.Unix.csand it's threadsSocketAsyncEngine.Wasi.csand integrated it with pollables and main event looplibcallows C-code linked with the samelibcto PInvoke SafeHandle and share itAlternative design
wasi-libcemulation, directly on top ofwasi-socketsAPI.Unit tests
IsThreadingSupportednow[SkipOnPlatform], for the APIs which I believe would not be fixed in the futureTODO
HAVE_SOCKADDR_UN_SUN_PATHand UnixDomainSocketEndPointRelated issues
Filled
wasi-libcissues