search

apple / swift-system

Low-level system calls and types for Swift
Apache License 2.0
1.18k stars 102 forks source link

Add Sendable conformance to FileDescriptor #112

Open ffried opened 1 year ago

ffried commented 1 year ago

This adds Sendable conformance to FileDescriptor alongside the other public types (see #115).

milseman commented 1 year ago

I'd like to reengage with this topic. I'm still (even more) on the side of FileDescriptor being Sendable. @lorentey what would be the ABI impact of adding that conformance?

lorentey commented 1 year ago

Sendable has no ABI impact -- we can simply add the conformance whenever we are ready!

ffried commented 1 year ago

@milseman @lorentey Should I change this PR to add the conformance (w/o marking it as unavailable)?

milseman commented 1 year ago

Yes to adding it, deferring to @lorentey regarding availability.

ffried commented 1 year ago

In accordance with the change in 87e68fc, I've updated the conformance to be on the main type. This also resolves the conflicts that existed on this PR's branch.

Depending on @lorentey's decision regarding availability, I'll move it to an extension again (for marking it unavailable).

lorentey commented 3 weeks ago

Update: As a @frozen struct wrapping a single integer, the shipping FileDescriptor has already been implicitly inferred to be Sendable, as it should be.

Concurrent use of the same file descriptor does not lead to undefined behavior. And even if it did, it is not in Swift System's mandate to protect against that.

The job of Swift System is merely to present the preexisting C APIs in a way that feels native to Swift. This codebase is not in the business of inventing nontrivial abstractions over the constructs provided by the operating system, or of building pretend obstacles against their use. One can pass integer values between concurrent execution contexts and directly call the read/write/close/etc. syscalls on them -- therefore, it must be possible to do the same on FileDescriptor instances and their operations.

ffried commented 3 weeks ago

@lorentey Thanks for the update. In this case this PR should be ready as is, right? I've just rebased the branch on main to make sure there are no conflicts.

lorentey commented 3 weeks ago

@swift-ci test