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.

Sign up for GitHub

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

Implement FileDescriptor.Pipe() #58

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
fd1e85e
Add Pipe functionality
GeorgeLyon Jul 21, 2021
e0d8759
Address feedback
GeorgeLyon Aug 5, 2021
6a99c59
Revert to more verbose names for pipe ends
GeorgeLyon Aug 30, 2021
c51b1d1
Use inlet/outlet terminology
GeorgeLyon Aug 31, 2021
af3ab71
Remove Pipe struct
GeorgeLyon Sep 2, 2021
534e55f
Merge branch 'main' into dev/george/pipe
GeorgeLyon Sep 2, 2021
18b66f9
Swap input and output
GeorgeLyon Sep 7, 2021
1b37611
No need to retry on interrupt
GeorgeLyon Sep 7, 2021
b7bb636
Update Sources/System/Internals/Syscalls.swift
GeorgeLyon Sep 16, 2021
64a60aa
Apply suggestions from code review
GeorgeLyon Sep 20, 2021
a210eda
Update Tests/SystemTests/FileOperationsTest.swift
milseman Sep 21, 2021
eec2311
Update Tests/SystemTests/FileOperationsTest.swift
milseman Sep 21, 2021
48d4d94
Update Tests/SystemTests/FileOperationsTest.swift
milseman Sep 21, 2021
3c85094
Update Tests/SystemTests/FileOperationsTest.swift
milseman Sep 21, 2021
beaeff2
Disable pipe on Windows
GeorgeLyon Sep 24, 2021
e80e39c
Update Sources/System/FileOperations.swift
milseman Sep 27, 2021
86ad6f9
Update Sources/System/FileOperations.swift
milseman Sep 27, 2021
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
24 changes: 24 additions & 0 deletions Sources/System/FileOperations.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,4 +370,28 @@ extension FileDescriptor {
public func dup2() throws -> FileDescriptor {
fatalError("Not implemented")
}

#if !os(Windows)
/// Create a pipe, a unidirectional data channel which can be used for interprocess communication.
///
/// - Returns: The pair of file descriptors.
///
/// The corresponding C function is `pipe`.
@_alwaysEmitIntoClient
// @available(macOS 9999, iOS 9999, watchOS 9999, tvOS 9999, *)
public static func pipe() throws -> (readEnd: FileDescriptor, writeEnd: FileDescriptor) {
try _pipe().get()
}

@usableFromInline
internal static func _pipe() -> Result<(readEnd: FileDescriptor, writeEnd: FileDescriptor), Errno> {
var fds: (Int32, Int32) = (-1, -1)
return withUnsafeMutablePointer(to: &fds) { pointer in
valueOrErrno(retryOnInterrupt: false) {
system_pipe(UnsafeMutableRawPointer(pointer).assumingMemoryBound(to: Int32.self))
}
}.map { _ in (.init(rawValue: fds.0), .init(rawValue: fds.1)) }
}
Copy link
Contributor

@milseman milseman Sep 20, 2021 •
edited
Loading

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 •
edited
Loading

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
}
#endif
}
8 changes: 8 additions & 0 deletions Sources/System/Internals/Syscalls.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,11 @@ internal func system_dup2(_ fd: Int32, _ fd2: Int32) -> Int32 {
#endif
return dup2(fd, fd2)
}
#if !os(Windows)
internal func system_pipe(_ fds: UnsafeMutablePointer<Int32>) -> CInt {
#if ENABLE_MOCKING
if mockingEnabled { return _mock(fds) }
#endif
return pipe(fds)
}
#endif
21 changes: 21 additions & 0 deletions Tests/SystemTests/FileOperationsTest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,27 @@ final class FileOperationsTest: XCTestCase {
func testHelpers() {
// TODO: Test writeAll, writeAll(toAbsoluteOffset), closeAfter
}

#if !os(Windows)
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.readEnd.closeAfter {
try pipe.writeEnd.closeAfter {
var abc = "abc"
try abc.withUTF8 {
_ = try pipe.writeEnd.write(UnsafeRawBufferPointer($0))
}
let readLen = 3
let readBytes = try Array<UInt8>(unsafeUninitializedCapacity: readLen) { buf, count in
count = try pipe.readEnd.read(into: UnsafeMutableRawBufferPointer(buf))
}
XCTAssertEqual(readBytes, Array(abc.utf8))
}
}
}
#endif

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