Introduce AsyncIO for Windows and Linux

[iCharlesHu]

This PR finishes AsyncIO by introducing Windows IO Completion Port based implementation.

This PR is staged on top of #64 and should be considered ready to be review as soon as that one lands.

Resolves: #105

[iCharlesHu]

Resolves: #110

[itingliu]

Talked to @iCharlesHu offline and he confirmed that if we merge this one, we won't need #64

[jakepetroules]

I would be a bit worried about running into collisions here, there is a good chance this could repeat and lead to subtle bugs. Instead of making it random, what about using a monotonically increasing unsigned 64-bit integer stored in a static variable? Then we can guarantee this is never a problem.

Something like this maybe?

func nextPipeNumber() -> UInt64 {
    struct Static: ~Copyable {
        static let pipeNumber = Atomic<UInt64>(0)
    }
    return Static.pipeNumber.add(1, ordering: .sequentiallyConsistent).newValue
}

[iCharlesHu]

How about UUID? It feels heavy to introduce lock and atomics just for a random value.

Also: is this really a concern? I would be really worried if a random number between .min and .max starts to repeat fairly soon.

[iCharlesHu]

Never mind UUID is not available in Subprocess. I still feel like the chances of us running into collisions are fairly low, and introducing atomic just for this purpose doesn't seem to justify the additional safety it brings.

@cthielen do you have any thoughts on this?

[iCharlesHu]

I believe random number collisions follows the Birthday Paradox rule, which according to (multiple) ChatGPT/Claude calculations puts us around 3.5 ^ 10 ^9 range for full 64 bit signed int. This means we have to run 3.5 * 10^9 times for it to have more than 50% chance repeating. To me that's statistically negligible.

[jakepetroules]

Why is using an Atomic for this "heavy"? (Also, not a lock and atomics -- just an atomic. No need for a lock as well) How do you know Int.random is not actually more expensive? I did a quick benchmark and atomics seem to outperform Int.random by an order of magnitude...

import Synchronization

let pipeNumber = Atomic<UInt64>(0)

func main() {
    let clock = ContinuousClock()
    let count = 100_000
    let random = clock.measure {
        for _ in 0..<count {
            Int.random(in: .min ..< .max)
        }
    }

    let atomic = clock.measure {
        for _ in 0..<count {
            pipeNumber.add(1, ordering: .sequentiallyConsistent).newValue
        }
    }

    print("random = \(random)")
    print("atomic = \(atomic)")
}

main()

random = 0.0332772 seconds (33.2 ms)
atomic = 0.0006226 seconds (0.6 ms)

[al45tair]

Atomics aren't sufficient here. Named pipe names are system-wide.

Why are we using a named pipe here anyway? What's wrong with CreatePipe()? Using a Windows named pipe is an odd thing to do unless you want to open it from some other process (or across the network).

If we absolutely must use a named pipe for some reason, we should be trying to create it in a loop, with the CreateNamedPipe function's dwOpenMode set to include FILE_FLAG_FIRST_PIPE_INSTANCE, and then adjust the name on each pass through the loop so that we get a unique name. An additional gotcha here is that in some circumstances named pipes are restricted to the same container and must start their names with "\\.\pipe\LOCAL\". I think it's very likely the wrong choice for this code…

[weissi]

Named pipe names are system-wide

I'm glad you looked over this. Had no idea this is a named pipe :P.

[iCharlesHu]

@al45tair we have to use named pipe here because Microsoft's documentation states that:

Asynchronous (overlapped) read and write operations are not supported by anonymous pipes.

See https://learn.microsoft.com/en-us/windows/win32/ipc/anonymous-pipe-operations

[al45tair]

Ah, good point, though the actual problem is really just that CreatePipe() doesn't have a flags argument for you to specify FILE_FLAG_OVERLAPPED. Interestingly, Windows apparently used to use a named pipe anyway to implement anonymous pipes (see https://stackoverflow.com/questions/60645/overlapped-i-o-on-anonymous-pipe).

[iCharlesHu]

@al45tair thanks for linking that! Thanks to @jakepetroules's counter suggestion our current implementation of CreatedPipe is (almost) the same as the one listed above.

[jakepetroules]

Oh, one more suggestion, could AsyncIO.swift be split up into one file per backend? IOCP, epoll, Dispatch. That would make it easier to add others in future, like kqueue (BSD), poll (non-Linux non-BSD Unix), without the one file getting too big.

[cthielen]

Is there an advantage to this over UInt64(biPattern:)?

[iCharlesHu]

UInt64(biPattern:) requires Int64 so I had to convert twice. Otherwise I get the error message error: cannot convert value of type 'HANDLE' (aka 'UnsafeMutableRawPointer') to expected argument type 'Int64'

[weissi]

this will go badly wrong if we do this more than once. shutdown should protect against that. After pthread_join the pthread_t is a dangling pointer

[iCharlesHu]

shutdown is only called in atexit, which by definition should only run once.

[weissi]

do we have a guarantee that fileDescriptor is not an actual file? Because on actual files, epoll will always return writable even if you can't write them and write will block (e.g. on NFS or other network FSes).

[iCharlesHu]

Yes. The only scenario that FileDescriptor might be a file is when user pass in FD directly. In those cases we bind them to child process directly and we don't read from / write to them.

[weissi]

this whole file has very high complexity code that seems fairly adhoc, do we have a rigorous test suite for this?

[iCharlesHu]

fairly adhoc

How so? Is there a more canonical way of doing AsyncIO on Linux? This new implementation passes all existing tests and I'll be adding more later: #97

We need to finish up the implementation first.

[weissi]

Using epoll/kqueue correctly is notoriously difficult, so typically you would create an abstraction for it and rigorously test that. For example, NIO has a Selector (https://github.com/apple/swift-nio/blob/main/Sources/NIOPosix/SelectorGeneric.swift) implementation which abstracts a lot of the difficult stuff.

Not abstracting that usually leads to very-hard-to-debug stalls or CPU spins etc.

[iCharlesHu]

Removed DispatchData._ContiguousBufferView

[jakepetroules]

This is coming along really nicely! It looks pretty good to me at this point, and the last thing I'd like to see addressed before I'd feel comfortable giving a formal ✅ is the @sendable issues with DispatchIO detailed in #117 (comment)

[iCharlesHu]

Resolves: #105

[jakepetroules]

Sticking the @unchecked @Sendable on the DisptatchIO handler as discussed would be a nice final bit of polish, but I think this is ready to land either way from my perspective.

[iCharlesHu]

Sticking the @unchecked @Sendable on the DisptatchIO handler as discussed would be a nice final bit of polish, but I think this is ready to land either way from my perspective.

Unfortunately, you can't use @unchecked @Sendable on closures, only types -- this is largely why I decided to not bother now. We'd have to come up with some hack that involves us moving that closure to some type with @unchecked Sendable, which I'm not sure it will work until we see the actual error message.

[iCharlesHu]

Appears to also resolve: #119

[al45tair]

OK, one final thing about this code (sorry). I think we should at least add PIPE_REJECT_REMOTE_CLIENTS to the flags.

Ideally I think we'd specify an ACL for the named pipe, to make it harder to interfere with this code by having some other process call CreateFile between the CreateNamedPipeW and CreateFileW calls here, but the window for mischief is quite small and PIPE_REJECT_REMOTE_CLIENTS would at least restrict it to things already running on the current machine.