Add opt-in API for channels to expose their underlying transport#3509
Conversation
Co-authored-by: Agam Dua <agam_dua@apple.com>
…able via syncOperations
765e0da to
ccc1b15
Compare
| /// Not all channels support access to the underlying channel. If the channel does not support this API, the | ||
| /// closure is not called and this function immediately returns `nil`. |
There was a problem hiding this comment.
Note: This matches the semantics of Collection.withContiguousStorageIfAvailable(_:) w.r.t. to if the type doesn't support the operation, i.e. the closure is not called and the optional generic return type is used to signal that.
0037d00 to
0334941
Compare
simonjbeaumont
left a comment
There was a problem hiding this comment.
@Lukasa OK, this one is now ready for a re-review. PTAL 🙏
| // Calling without explicit transport type does not run closure, even if body uses compatible literal value. | ||
| try #expect(syncOps.withUnsafeTransportIfAvailable { transport in transport != -1 } == nil) |
There was a problem hiding this comment.
This one is probably the sharpest edge of this design.
There was a problem hiding this comment.
I think this edge isn't as sharp as it looks. The problem here is that the literal ends up being inferred to be of type Int, which isn't CInt, because there is no type information to suggest a better choice. If you wrote -1 as CInt or CInt(-1) then you'll find this works correctly.
There was a problem hiding this comment.
Yes, I understood why it didn't work. I just think that this isn't going to be super obvious for adopters, but It think it's the best we can do and this is an off-the-beaten path, advanced API, so I'm OK with it if you are.
There was a problem hiding this comment.
Maybe it's even less surprising than the closure that excludes the argument not being run. But again, I think this is an OK trade.
0334941 to
70a59e4
Compare
70a59e4 to
3bfafdb
Compare
| } | ||
| } | ||
|
|
||
| extension BaseSocketChannel: NIOTransportAccessibleChannelCore where SocketType: BaseSocket { |
There was a problem hiding this comment.
We should probably do this with `PipeChannel while we're here.
There was a problem hiding this comment.
Heh, this is the channel I used to test the behaviour when this API isn't implemented 😆.
I'll have to think of something else for that, maybe EmbeddedChannel?
There was a problem hiding this comment.
OK, I've pushed a new commit that refactors how we do the conformance so that we can get conformances on other channels, including pipe channel. And I've added tests that all the channels you can get back from a bootstrap conform.
Motivation
NIO channels abstract over an underlying transport mechanisms (sockets, pipes, etc.), which users typically need not interact with. However, there are scenarios where users need direct access to the underlying transport for low-level operations that work outside NIOs abstraction. One example is performing out out-of-band operations on the underlying file descriptor for a socket-based channel.
This PR adds a structured way to access the underlying transport of a channel, for channels that choose to implement it.
Modifications
public protocol NIOTransportAccessibleChannelCore<Transport>which provides a scopedwithUnsafeTransport(_:), used for channel implementations to opt-in.NIOTransportAccessibleChannel<NIOBSDSocket.Handle>forBaseSocketChannel, to make this API available for all socket-based channels, including channels returned from the socket-based bootstrap public APIs.ChannelPipeline.SynchronousOperations.withUnsafeTransportIfAvailable(_:)Note that not all channels need to or should their transport, which is why this was added as an additional protocol that refines
ChannelCore, vs. extendingChannelorChannelCorewith a default implementation.The protocol uses a primary associated type allowing channels to provide typed access to the transport. E.g. this could be used in NIO Transport Services to expose the underlying
NWConnection, if desired.The method itself is spelled with "unsafe", uses scoped access, and has clear documentation that users must not violated any of NIOs assumptions about the state of the underlying transport. It's very much not intended for every day use. It being on
ChannelCoreshould defer users of the low-level API, sinceChannel._channelCoreis marked as for NIO internal use, and the publicwithUnsafeTransportIfAvailable(_:)will take care of the runtime checks for channels that have opted into this API.Result
ChannelPipeline.SynchronousOperations.withUnsafeTransportIfAvailable(_:)for users