apple / swift-corelibs-foundation

The Foundation Project, providing core utilities, internationalization, and OS independence
swift.org
Apache License 2.0
5.23k stars 1.12k forks source link

[Windows] Use `fd_set` according to Winsock2 specifics #4954

Closed lxbndr closed 1 month ago

lxbndr commented 1 month ago

Fixes https://github.com/apple/swift/issues/73532.

On Windows, socket handles in a fd_set are not represented as bit flags as in Berkeley sockets. While we have no fd_set dynamic growth in this implementation, the FD_SETSIZE defined as 1024 in CoreFoundation_Prefix.h should be enough for majority of tasks.

This also includes a test for SocketPort which uses CFSocket internally. This test fails (hangs or crashes with memory corruption error) without the fix.

lxbndr commented 1 month ago

@hjyamauchi this patch has minor differences from that version you checked. Just to be sure, could you please validate original issue with this implementation?

parkera commented 1 month ago

@swift-ci test

lxbndr commented 1 month ago

Force-pushed NFC (removed unused variable x in the for-loop in the test code).

compnerd commented 1 month ago

@swift-ci please test

hjyamauchi commented 1 month ago

@hjyamauchi this patch has minor differences from that version you checked. Just to be sure, could you please validate original issue with this implementation?

Validated. It worked fine.

hjyamauchi commented 1 month ago

CC @tristanlabelle

lxbndr commented 1 month ago

@tristanlabelle so the major concern is the static non-growing structure, right? I agree that it is not comfortable to live with this restriction, and my initial intention was to make it dynamic.

Not sure if it is really ok to manipulate inner structure array, but I think it worth to try. I am suspicious about changing it on the fly, because FD_SETSIZE is a define, and it must be defined before including winsock2 header. So it could be baked into some macros like FD_SET or FD_ZERO.

I initially planned not to grow the array, but create additional fd_sets as needed, so we would have growing array of fd_sets instead.

Also, some good news. The default cap is 64, which is pretty low. But look what we got here. We already have FD_SETSIZE defined as 1024. That's a plenty of space.

I could try dynamic sizing, but probably not early than this weekend 😔 If that not blocking anything critical for you, we could keep this for now. Or we could land it and I'll do a follow up.

And... Do you think it makes sense to fight 1024 sockets limit, while corelibs are gradually moving away from CoreFoundation as backing implementation? WDYT?

tristanlabelle commented 1 month ago

@lxbndr Ah given the 1024 socket override I don't have much of a concern. I'll resolve my comments and file a follow-up issue.

You're right that we wouldn't be able to use FD_SET or FD_ZERO on Windows with dynamic sizing, they would need their own implementation.

tristanlabelle commented 1 month ago

Actually FD_SET is the only problematic one for dynamic sizing:


#define FD_CLR(fd, set) do { \
    u_int __i; \
    for (__i = 0; __i < ((fd_set FAR *)(set))->fd_count ; __i++) { \
        if (((fd_set FAR *)(set))->fd_array[__i] == fd) { \
            while (__i < ((fd_set FAR *)(set))->fd_count-1) { \
                ((fd_set FAR *)(set))->fd_array[__i] = \
                    ((fd_set FAR *)(set))->fd_array[__i+1]; \
                __i++; \
            } \
            ((fd_set FAR *)(set))->fd_count--; \
            break; \
        } \
    } \
} while(0)

#define FD_SET(fd, set) do { \
    if (((fd_set FAR *)(set))->fd_count < FD_SETSIZE) \
        ((fd_set FAR *)(set))->fd_array[((fd_set FAR *)(set))->fd_count++]=(fd);\
} while(0)

#define FD_ZERO(set) (((fd_set FAR *)(set))->fd_count=0)

#define FD_ISSET(fd, set) __WSAFDIsSet((SOCKET)(fd), (fd_set FAR *)(set))
tristanlabelle commented 1 month ago

Filed https://github.com/apple/swift-corelibs-foundation/issues/4958 as follow-up

compnerd commented 1 month ago

@swift-ci please test Windows platform

compnerd commented 1 month ago

@swift-ci please test macOS platform

hjyamauchi commented 1 month ago

@swift-ci please test macOS platform

hjyamauchi commented 1 month ago

Does it looks like the macos CI was broken on May 11 before this PR was created? The latest run 466 looks broken in the same way as the May 11 run 461.

May 20 (the latest): https://ci.swift.org/job/swift-corelibs-foundation-PR-macOS/466/

May 11: https://ci.swift.org/job/swift-corelibs-foundation-PR-macOS/461/

Failed Tests (24):
  SwiftXCTestFunctionalTests :: ArgumentParser/main.swift
  SwiftXCTestFunctionalTests :: Asynchronous/Expectations/main.swift
  SwiftXCTestFunctionalTests :: Asynchronous/Handler/main.swift
  SwiftXCTestFunctionalTests :: Asynchronous/Misuse/main.swift
  SwiftXCTestFunctionalTests :: Asynchronous/Notifications/Expectations/main.swift
  SwiftXCTestFunctionalTests :: Asynchronous/Notifications/Handler/main.swift
  SwiftXCTestFunctionalTests :: Asynchronous/Predicates/Expectations/main.swift
  SwiftXCTestFunctionalTests :: Asynchronous/Predicates/Handler/main.swift
  SwiftXCTestFunctionalTests :: Asynchronous/Use/main.swift
  SwiftXCTestFunctionalTests :: EqualityWithAccuracy/main.swift
  SwiftXCTestFunctionalTests :: ErrorHandling/main.swift
  SwiftXCTestFunctionalTests :: FailingTestSuite/main.swift
  SwiftXCTestFunctionalTests :: FailureMessagesTestCase/main.swift
  SwiftXCTestFunctionalTests :: ListTests/main.swift
  SwiftXCTestFunctionalTests :: Observation/All/main.swift
  SwiftXCTestFunctionalTests :: Observation/MultipleObservers/main.swift
  SwiftXCTestFunctionalTests :: Observation/Selected/main.swift
  SwiftXCTestFunctionalTests :: Performance/Misuse/main.swift
  SwiftXCTestFunctionalTests :: Performance/main.swift
  SwiftXCTestFunctionalTests :: SelectedTest/main.swift
  SwiftXCTestFunctionalTests :: SingleFailingTestCase/main.swift
  SwiftXCTestFunctionalTests :: SkippingTestCase/main.swift
  SwiftXCTestFunctionalTests :: TestCaseLifecycle/Misuse/main.swift
  SwiftXCTestFunctionalTests :: TestCaseLifecycle/main.swift
compnerd commented 1 month ago

This is really windows specific, the tests haven't regressed, @parkera okay to merge ignoring the macOS issue?