1

I'm currently trying to do some rudimentary processing with a NIO Pipe channel. I've set up a reduced test case here, where I do two things: First, I simply invoke "echo" using Foundation.Process, and read the output using a Foundation.Pipe; Second, I perform the same task, but I pipe the output to the input of a NIOPipeBootstrapped channel with a channel handler which just writes any read data to the channel output (theoretically a noop). I then read the output of the channel using a Foundation.Pipe.
The first case works as I would expect, and each time the read data matches what was echoed. The NIO case, however, mostly matches the input, though occasionally produces empty output and occasionally throws an error (NIO.ChannelError.ioOnClosedChannel).
The NIO code is pretty simple, I'm wondering if anyone has some idea what I'm doing wrong:
https://github.com/GeorgeLyon/NIOProcessExample/blob/cbaca51619b4eb83446e144310d56b2ae385e2be/Sources/NIOProcessExample/main.swift#L47-L66

2

Ah, I'm pretty sure I know what's going on here. Sadly, you fell into the cracks between two APIs here.

SwiftNIO's NIOPipeBootstrap operates on raw file descriptors, ie. it can't enforce the lifecycle for you. SwiftNIO expects (see docs) that you transfer ownership of the file descriptors you hand to SwiftNIO. In other words: You guarantee that you do no further operations on the file descriptors that you pass to SwiftNIO.

In your program it looks like you're not violating that rule, however Foundation's Pipe will call close on its file descriptors once the Pipe's reference count decreases to 0. That means, you Pipe will still do operations on file descriptors that you passed to NIO which violates the above contract.

Fortunately, the fix is very simple: If you change your code to

let pipe = Pipe()

// Just two variables to make it easier to read
let inputFileHandle = handle
let outputFileHandle = pipe.fileHandleForWriting

let channel = try NIOPipeBootstrap(group: group)
  .withPipes(
    // note the `dup`s here, we duplicate the fds
    // before handing them to NIO. So NIO exclusively
    // owns and closes these fds.
    inputDescriptor: dup(inputFileHandle.fileDescriptor),
    outputDescriptor: dup(outputFileHandle.fileDescriptor))
  .wait()
try channel.pipeline.addHandler(Handler()).wait()

// We can now close the originals because NIO has its
// own duplicates of these
inputFileHandle.closeFile()
outputFileHandle.closeFile()

return String(
  data: pipe.fileHandleForReading.readDataToEndOfFile(),
  encoding: .utf8)!

You can find similar code in NIO's crash tester.

What we should do (and be very happy to accept a PR for if you're interested :) ) would be to create another withFileHandles or so in the NIOFoundationCompat module that accepts Foundation.FileHandles instead of raw fds. That can then (inside of NIO) to this dup and closeFile dance. That should make it easier and less error-prone for everybody to interface between Foundation.FileHandle/Foundation.Pipe and NIO.PipeBootstrap.

3

Oops, and I forgot one more rather important point: SwiftNIO at its heart is a network programming framework. And despite the fact that TCP supports half-closure (one direction of the connection is still open, the other one closed), most network programmers don't actually want to deal with half-closure. Therefore, SwiftNIO (like most other networking frameworks) by default maps a closure of the input (ie. what it reads from the remote end) to a full closure of the Channel.

I think, this is probably a sensible default for TCP connections because most programmers think of a TCP connection as either being open or closed (as opposed to fully open, closed, input closed (& output open), or output closed (& input open)).

When using SwiftNIO with pipes however, it's quite questionable whether that's a good default. I'm probably responsible for making NIOPipeBootstrap default to not supporting remote half-closure. I guess my thinking was to match the other bootstraps but I now think that was probably the wrong choice.

Long story short, you want a .channelOption(ChannelOptions.allowRemoteHalfClosure, value: true) to enable half-closure. And once you do that, your Channel will no longer automatically close if we read an EOF. Do however remember to context.close(promise: nil) that Channel once you're done with it, for example when that writeAndFlush succeeded.

let channel = try NIOPipeBootstrap(group: group)
   .channelOption(ChannelOptions.allowRemoteHalfClosure, value: true)
   .withPipes(...)
   ...

Addendum (through edits):

  • I filed an issue about the probably bad allowRemoteHalfClosure default in NIOPipeBootstrap
  • My first response is still stands but I don't think that's the issue you're hitting here because you wouldn't be getting .ioOnClosedChannel
  • The reason you get .ioOnClosedChannel is because the input is already shut down when we can issue the write. But without .allowRemoteHalfClosure: true, the whole Channel gets shut down when the input reads EOF.
4

Thanks for the quick and thorough responses!

For posterity, the file handle ownership issue had no effect on the output: https://github.com/GeorgeLyon/NIOProcessExample/commit/84ec0e80376c25fc02abdb0f2381f7f70318b8a5

allowRemoteHalfClosure, on the other hand, did fix the issue mostly, though I also had a bug where I could start sending data to the pipeline before the channel handler was added to the pipeline (update: I've fixed this better(?) by turning autoRead off: https://github.com/GeorgeLyon/NIOProcessExample/commit/b52ec1c7da1b686e2f273e8e4523b76a7d0a6edd). For those curious, the diff is here: https://github.com/GeorgeLyon/NIOProcessExample/commit/f2c38c83f678ffae5229e095a77fdde193c8ef36

One additional thing I needed to do to make this work was to manually close the output context. I'm curious whether this is the correct way to do so:
https://github.com/GeorgeLyon/NIOProcessExample/blob/f2c38c83f678ffae5229e095a77fdde193c8ef36/Sources/NIOProcessExample/main.swift#L54-L59
I'm skeptical because channelReadComplete fires twice in this example, so I had to store extra state (closed) to determine whether or not to close the output, and I couldn't find a way to query for whether or not a channel was closed.

5

Glad we got this fixed. Yeah, I think the fd ownership issue didn't matter yet because the Pipe didn't deinit before you hit the other issue.

Probably not. As you know, channelRead delivers you the data read. But NIO can't predict how much data is available in the kernel, so SwiftNIO may need to read it in multiple attempts. channelReadComplete marks the end of one of those "read bursts". So you'll need to expect many channelReadCompletes.

You could implement

    func userInboundEventTriggered(context: ChannelHandlerContext, event: Any) {
        defer {
            context.fireUserInboundEventTriggered(event)
        }
        if case .inputClosed = event as? ChannelEvent {
            // we read EOF, ie. the input is now closed
            self.inputClosed = true
        }
    }

in your ChannelInboundHandler/ChannelDuplexHandler, that way you can learn that your input was closed.

Probably details but likely you'll want context.close(promise: nil) here (which implies mode: .all). I'd only use context.close(mode: .output) if you really want to keep the input alive. Ie. context.close(mode: .output) only makes sense if you really need to send an EOF to the other side without also closing your input.

Ugh, are you adding the handler in the channelInitializer? If yes, this could be a bug in NIO, do you have a repro for this?

I'd recommend against switching autoRead off. It's almost certainly not going to do what you want. For all stream oriented transports like UNIX pipes and TCP, you cannot rely the framing of your data, so assuming the number of reads you'll need to read all the data won't really work.

Digression: framing, feel free to skip if you know that already...

In other words, say one end sends "hello world", there are no guarantees that you will read this as "hello world". You may read it for example as "hello" followed by a " world". And in the same way, if the other end sends "hello world" followed by "it's cold today". You may read it as "hello worldit's cold today". There's nothing SwiftNIO can do here, that's just how stream-oriented transports work.

To fix this, most (network) protocols implement some form of framing. In the UNIX context, "line framing" is often used. Ie. we process one line at a time. So if you send "hello world\n" followed by "it's cold today\n", the other side may receive that as "hello world\nit's cold today\n" but it can still restore the original framing. Other, more common ways of framing is to prepend a length before each message. SwiftNIO works really great with this because you can add a ChannelHandler that takes care of the framing and after that, your pipeline works on messages and no longer on a stream of bytes. So you can totally forget about the fact that you're on a stream-based transport. In swift-nio-extras we have for example LineBasedFrameDecoder which can "decode" newline framed messages. Similarly LengthFieldBasedFrameDecoder which does the same for length-prefixed messages.

There are multiple reasons why I'd recommend against autoRead = false but the strongest in your case is that you'd then rely on all of your data arriving in a single read. This may work in your testing but may well fail at a random point in time, you can't control how the kernel (and the other end) work.

2 Likes
6

So you are saying .all will do the right thing if the input is already closed, cool.

I'm not, in my quest to make a reduced test case I moved from adding the handler in an initializer to just adding it after the channel was created. That was a bad idea.

I think this makes sense from what the project shows, but where I am actually using this code, I do have some notion of framing and I in fact what to explicitly stop reading if reading frames outpaces my ability to process them. What you say makes sense if someone had a different use case.

Thanks again for all the help!

7

.all is always acceptable and that will make sure the Channel is fully torn down.

That should still work assuming you order things correctly. And I can see in your case how autoRead = false may have helped here.

What you could use is

let channel = NIOPipeBootstrap(...)
    // autoRead = false so that no data is read before handler is in
    .channelOptions(ChannelOptions.autoRead, value: false)
    ....

// add handler
channel.pipeline.addHandler(...).wait()

// set auto-read = true, so that data can now flow
channel.setOption(ChannelOptions.autoRead, value: true).wait()

This will work, but I'd very much recommend leaving autoRead alone and adding your handler in the channelInitializer.

You do have enough control over everything in NIO that you don't technically need to do that but I'd recommend against it :slight_smile:. Feel free to post more here if there are other bugs that you can't fix without chaning autoRead and using the channelInitializer.

1 Like