Skip to content
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

Implement FileDescriptor.Pipe() #58

Merged
merged 17 commits into from
Sep 27, 2021
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions Sources/System/FileOperations.swift
Original file line number Diff line number Diff line change
Expand Up @@ -370,4 +370,37 @@ extension FileDescriptor {
public func dup2() throws -> FileDescriptor {
fatalError("Not implemented")
}

/// A pair of `FileDescriptor` values represening a pipe.
///
/// A pipe enables data written to `output` to be read from `input`.
/// You are responsible for managing the lifetime and validity
/// of the `input` and `output` `FileDescriptor` values.
public typealias Pipe = (input: FileDescriptor, output: FileDescriptor)

GeorgeLyon marked this conversation as resolved.
Show resolved Hide resolved
/// Create a pipe, a uniderctional data channel which can be used for interprocess communication.
GeorgeLyon marked this conversation as resolved.
Show resolved Hide resolved
///
/// - Parameters:
/// - retryOnInterrupt: Whether to retry the write operation
/// if it throws ``Errno/interrupted``. The default is `true`.
/// Pass `false` to try only once and throw an error upon interruption.
GeorgeLyon marked this conversation as resolved.
Show resolved Hide resolved
/// - Returns: The new file descriptor.
GeorgeLyon marked this conversation as resolved.
Show resolved Hide resolved
///
/// The corresponding C function is `pipe`.
@_alwaysEmitIntoClient
// @available(macOS 9999, iOS 9999, watchOS 9999, tvOS 9999, *)
public static func pipe() throws -> Pipe {
GeorgeLyon marked this conversation as resolved.
Show resolved Hide resolved
try _pipe().get()
}

@usableFromInline
internal static func _pipe() -> Result<Pipe, Errno> {
GeorgeLyon marked this conversation as resolved.
Show resolved Hide resolved
var fds: (Int32, Int32) = (-1, -1)
return withUnsafeMutableBytes(of: &fds) { bytes in
let fds = bytes.bindMemory(to: Int32.self)
return valueOrErrno(retryOnInterrupt: false) {
system_pipe(fds.baseAddress!)
}.map { _ in (FileDescriptor(rawValue: fds[0]), FileDescriptor(rawValue: fds[1])) }
milseman marked this conversation as resolved.
Show resolved Hide resolved
}
Copy link
Contributor

@milseman milseman Sep 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might be able to say var fds: Array<CInt> = [-1, -1] and use the implicit array-to-pointer conversion by passing &fds to the syscall (though I don't recall if it still works or has limitations). Otherwise, you can probably use withUnsafeMutablePointer to avoid the extra bind-memory step.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lorentey suggested I use the tuple form for a stronger guarantee that those values would end up on the stack. I haven't checked but I believe withUnsafeMutablePointer will still require rebinding the type (I think this was a signedness issue), but would also require manually passing the count (1), whereas withUnsafeMutableBytes takes this information from the buffer pointer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do make sure that this builds on windows - I'm almost certain that this will break the Windows builds. Also note that Windows should transact in HANDLEs, aka void *, so this is going to truncate at least. See CreatePipe.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it's fine to simply surround these additions with #if !os(Windows) for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great! @atrick / @glessard , what's the lesser of evils for the pointer binding stuff?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pointer to a homogenous tuple is also bound to its element type, so you can use assumingMemoryBound.

(I hope. I use this quite a lot)

Copy link
Contributor

@glessard glessard Sep 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As written, this case indeed calls for assumingMemoryBound.
The memory is already bound to Int32, so bindMemory doesn't add information.

This being said, we unfortunately don't have assumingMemoryBound on the RawBufferPointer types at this point. We also gain nothing from using a Buffer in this case: we must rely in the C call to be well behaved instead of having any sort of bounds checking. Given that, I suggest this for lines 389-393:

    return withUnsafeMutablePointer(to: &fds) { pointer in
      valueOrErrno(retryOnInterrupt: false) {
        system_pipe(UnsafeMutableRawPointer(pointer).assumingMemoryBound(to: Int32.self))
      }
    }.map { _ in (FileDescriptor(rawValue: fds.0), FileDescriptor(rawValue: fds.1)) }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karl is correct, and @glessard 's code edit looks good.
Although the original code was harmless, it doesn't really make sense to bind memory that belongs to a variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a document detailing what "binding memory" actually does? I had thought this was just managing compile-time information so I'm not 100% clear on the difference between "assuming" memory bound and actually binding memory.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some scattered documentation, including the doc-comments, but the largest chunk is in the RawPointer proposal: https://github.com/apple/swift-evolution/blob/main/proposals/0107-unsaferawpointer.md
Experience has shown that those sources are insufficient.

@NevinBR (I think) came up with a nice description of binding for humans a few weeks ago: https://forums.swift.org/t/pitch-implicit-pointer-conversion-for-c-interoperability/51129/36.

In general, if you're reminding the compiler of type information it should already have known, but had been obscured for some reason, then assumingMemoryBound is the thing to reach for (as we did here). If you are telling the compiler new information, then reach for bindMemory or withMemoryRebound.

milseman marked this conversation as resolved.
Show resolved Hide resolved
}
}
7 changes: 7 additions & 0 deletions Sources/System/Internals/Syscalls.swift
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,10 @@ internal func system_dup2(_ fd: Int32, _ fd2: Int32) -> Int32 {
#endif
return dup2(fd, fd2)
}

internal func system_pipe(_ fds: UnsafeMutablePointer<Int32>) -> CInt {
#if ENABLE_MOCKING
if mockingEnabled { return _mock(fds) }
#endif
return pipe(fds)
}
19 changes: 19 additions & 0 deletions Tests/SystemTests/FileOperationsTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,25 @@ final class FileOperationsTest: XCTestCase {
func testHelpers() {
// TODO: Test writeAll, writeAll(toAbsoluteOffset), closeAfter
}

func testAdHocPipe() throws {
// Ad-hoc test testing `Pipe` functionality.
// We cannot test `Pipe` using `MockTestCase` because it calls `pipe` with a pointer to an array local to the `Pipe`, the address of which we do not know prior to invoking `Pipe`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably do (not gonna hold up this PR for it though) want to allow for mock testing of such stack local pointers. We'd probably have a variant that we'd pass in an array and it would compare the contents.

let pipe = try FileDescriptor.pipe()
try pipe.input.closeAfter {
milseman marked this conversation as resolved.
Show resolved Hide resolved
try pipe.output.closeAfter {
milseman marked this conversation as resolved.
Show resolved Hide resolved
var abc = "abc"
try abc.withUTF8 {
_ = try pipe.output.write(UnsafeRawBufferPointer($0))
milseman marked this conversation as resolved.
Show resolved Hide resolved
}
let readLen = 3
let readBytes = try Array<UInt8>(unsafeUninitializedCapacity: readLen) { buf, count in
count = try pipe.input.read(into: UnsafeMutableRawBufferPointer(buf))
milseman marked this conversation as resolved.
Show resolved Hide resolved
}
XCTAssertEqual(readBytes, Array(abc.utf8))
}
}
}

func testAdHocOpen() {
// Ad-hoc test touching a file system.
Expand Down