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

Add support for unidirectional NIOPipeBootstrap #2560

Merged
merged 2 commits into from Oct 24, 2023

Conversation

FranzBusch
Copy link
Contributor

Motivation

In some scenarios, it is useful to only have either an input or output side for a PipeChannel. This fixes #2444.

Modification

This PR adds new methods to NIOPipeBootstrap that make either the input or the output optional. Furthermore, I am intentionally breaking the API for the new async methods since those haven't shipped yet to reflect the same API there.

Result

It is now possible to bootstrap a PipeChannel with either the input or output side closed.

Tests/NIOPosixTests/AsyncChannelBootstrapTests.swift Outdated Show resolved Hide resolved
Tests/NIOPosixTests/AsyncChannelBootstrapTests.swift Outdated Show resolved Hide resolved
Tests/NIOPosixTests/AsyncChannelBootstrapTests.swift Outdated Show resolved Hide resolved
Tests/NIOPosixTests/AsyncChannelBootstrapTests.swift Outdated Show resolved Hide resolved
Tests/NIOPosixTests/AsyncChannelBootstrapTests.swift Outdated Show resolved Hide resolved
Tests/NIOPosixTests/AsyncChannelBootstrapTests.swift Outdated Show resolved Hide resolved
Tests/NIOPosixTests/AsyncChannelBootstrapTests.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/PipePair.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/PipePair.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/PipePair.swift Outdated Show resolved Hide resolved
@weissi
Copy link
Member

weissi commented Oct 18, 2023

Oh @FranzBusch let's also fix Sources/NIOCrashTester/main.swift which opens /dev/null (as devNull) just to be able to use the pipe boostrap.

@FranzBusch FranzBusch force-pushed the fb-one-direction-pipe-channel branch 2 times, most recently from 4172d79 to 55cae0a Compare October 18, 2023 14:04
@@ -22,7 +22,6 @@ internal struct OutputGrepper {

internal static func make(group: EventLoopGroup) -> OutputGrepper {
let processToChannel = Pipe()
let deadPipe = Pipe() // just so we have an output...
Copy link
Member

Choose a reason for hiding this comment

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

<3

Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Agree with weissi that we should drop the "s". I left a bunch of doc nits too... Otherwise looks good.

Sources/NIOPosix/Bootstrap.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/Bootstrap.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/Bootstrap.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/Bootstrap.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/Bootstrap.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/Bootstrap.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/Bootstrap.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

I left the same doc nit a bunch of times but also a bit confused by some of the naming... would be good to clear that up. Looks good otherwise!

Sources/NIOPosix/Bootstrap.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/Bootstrap.swift Show resolved Hide resolved
Sources/NIOPosix/Bootstrap.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/Bootstrap.swift Show resolved Hide resolved
Sources/NIOPosix/Bootstrap.swift Outdated Show resolved Hide resolved
@available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
func _takingOwnershipOfDescriptors<ChannelInitializerResult, PostRegistrationTransformationResult: Sendable>(
public func takingOwnershipOfDescriptor<Output: Sendable>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Output is a bit misleading in light of input/output descriptors. Should we use "Result" or something super-generic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are using Output in all the other methods some I am hesitant to change it just for this one. Also why do you suggest making this public?

Copy link
Contributor

Choose a reason for hiding this comment

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

We are using Output in all the other methods some I am hesitant to change it just for this one.

Yeah, I get that, it's probably fine as is.

Also why do you suggest making this public?

I didn't, you made it public!

Sources/NIOPosix/Bootstrap.swift Outdated Show resolved Hide resolved
# Motivation
In some scenarios, it is useful to only have either an input or output side for a `PipeChannel`. This fixes apple#2444.

# Modification
This PR adds new methods to `NIOPipeBootstrap` that make either the input or the output optional. Furthermore, I am intentionally breaking the API for the new async methods since those haven't shipped yet to reflect the same API there.

# Result
It is now possible to bootstrap a `PipeChannel` with either the input or output side closed.
@FranzBusch FranzBusch enabled auto-merge (squash) October 24, 2023 16:46
@FranzBusch FranzBusch merged commit 935dbdf into apple:main Oct 24, 2023
6 of 8 checks passed
@FranzBusch FranzBusch deleted the fb-one-direction-pipe-channel branch October 24, 2023 17:18
@FranzBusch FranzBusch added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NIOPipeBootstrap/PipeChannel: support only one direction
3 participants