Add support for WASILibc #2671
Add support for WASILibc #2671
Conversation
Dispatch is not supported on WASI, and only Unix domain sockets are supported, which means we have to exclude those APIs on this platform.
|
@swift-server-bot add to allowlist |
| @@ -151,7 +160,7 @@ let package = Package( | |||
| .target( | |||
| name: "NIOHTTP1", | |||
| dependencies: [ | |||
| "NIO", | |||
| .target(name: "NIO", condition: .when(platforms: historicalNIOPosixDependencyRequired)), | |||
There was a problem hiding this comment.
This condition should probably be persisted across almost any library target that depends on NIO which also depends on NIOCore.
_NIOConcurrency, NIOFoundationCompat, NIOWebSocket, NIOTLS appear to be the other places.
| @@ -45,7 +47,7 @@ public final class Lock { | |||
| public init() { | |||
| #if os(Windows) | |||
| InitializeSRWLock(self.mutex) | |||
| #else | |||
| #elseif !os(WASI) | |||
There was a problem hiding this comment.
I don't think we can merge a PR that silently disables these primitives on WASI without being extremely confident that we'll never need them. Is there any way we can replace this with a feature-detection on threading?
| @@ -199,12 +204,14 @@ extension NIOBSDSocket.ProtocolFamily { | |||
| public static let inet6: NIOBSDSocket.ProtocolFamily = | |||
| NIOBSDSocket.ProtocolFamily(rawValue: PF_INET6) | |||
|
|
|||
| #if !os(WASI) | |||
There was a problem hiding this comment.
My general preference here would be that the APIs here are present, they just end up failing when actually used in WASI. That would mean hardcoding the syscall value on WASI, or deliberately using a spoiler value. That note applies to the rest of this file.
| import Dispatch | ||
| #elseif canImport(WASILibc) |
There was a problem hiding this comment.
The elseif implies a relationship between these two ideas, but I think they're fundamentally unrelated.
| @@ -54,6 +57,7 @@ public final class NIOFileHandle: FileDescriptor { | |||
| assert(!self.isOpen, "leaked open NIOFileHandle(descriptor: \(self.descriptor)). Call `close()` to close or `takeDescriptorOwnership()` to take ownership and close by some other means.") | |||
| } | |||
|
|
|||
| #if !os(WASI) | |||
There was a problem hiding this comment.
Same note here: generally it's preferable to have APIs not simply be absent, but to misbehave when not present. In this case, duplicate will presumably become available, and it'd be nicer if this just threw instead. The only time that isn't my preference is when an API literally cannot be spelled, e.g. the return or argument types are missing.
Relatedly, it's best if we put the #if os at the lowest possible level. In this case, we should arrange for dup to throw or error out, and let the rest of the code path handle that appropriately, which minimises the OS customisation we have to do.
| @@ -445,6 +456,7 @@ public enum SocketAddress: CustomStringConvertible, Sendable { | |||
| self = .v6(.init(address: ipv6Addr, host: "")) | |||
| } | |||
|
|
|||
| #if !os(WASI) | |||
There was a problem hiding this comment.
Could you make makeAddressResolvingHost just throw exception instead of guarding out the API itself?
| @@ -140,6 +142,7 @@ public protocol SocketOptionProvider: _NIOPreconcurrencySendable { | |||
| // | |||
| // You are welcome to add more helper methods here, but each helper method you add must be tested. | |||
| extension SocketOptionProvider { | |||
| #if !os(WASI) | |||
There was a problem hiding this comment.
How about adding a typealias like the following instead of guarding out the whole method definition?
#if canImport(WASILibc)
typealias NIOLinger = Never
#else
typealias NIOLinger = linger
#endif
|
@MaxDesiatov / @Lukasa This looks like a PR that has bit rotting potential because it needs to touch a lot of pieces, most of the issues seem pretty tractable to me so I'd love to see this go in. w.r.t. disabling locks on WASI: Honestly I think this can go in as is: Today this is fine and if threading really happens this will blow up very obviously and can be fixed. |
Dispatch is not supported on WASI, and only Unix domain sockets are supported, which means we have to exclude those APIs on this platform.
There's work in progress to enable tests for this on CI, but nothing I can provide for this PR at the current moment.