Purging a promise circular buffer question

Related Projects SwiftNIO
1

Hi there,
in https://github.com/apple/swift-nio-extras/blob/master/Sources/NIOExtras/RequestResponseHandler.swift

    public func errorCaught(context: ChannelHandlerContext, error: Error) {
        guard self.state.isOperational else {
            assert(self.promiseBuffer.count == 0)
            return
        }
        self.state = .error(error)
        let promiseBuffer = self.promiseBuffer
        self.promiseBuffer.removeAll()
        context.close(promise: nil)
        promiseBuffer.forEach {
            $0.fail(error)
        }
    }

I notice this

let promiseBuffer = self.promiseBuffer

Can someone help to clarify the reason here using a temp struct?

Is it safe to write like

        self.state = .error(error)
        self.promiseBuffer .forEach { $0.fail(error) }
        self.promiseBuffer.removeAll()
        context.close(promise: nil)
        }

IMO, it should be safe to call like this.

thanks.

2

Because promises call out to arbitrary code.

The problem here is that when you satisfy a promise, the user's callback code runs immediately. That code may do any number of things, and one of those things might be to call Channel.write. If they do so, your ChannelHandler.write function will be called while you are still holding self.promiseBuffer mutably. You will then attempt to grab self.promiseBuffer mutably again, and Swift will crash due to an exclusive access violation.

Before you call any methods on ChannelHandlerContext or satisfy a promise you must arrange to ensure that your ChannelHandler can tolerate having any of its methods synchronously called, as that may happen. We are considering providing helpers to deal with this, but for now it's an unfortunate risk in SwiftNIO programs.

If you want to use more of a looping construct, you can write the code like this as well:

while self.promiseBuffer.count > 0 {
    self.promiseBuffer.removeFirst().fail(error)
}

This will also ensure that you are not holding promiseBuffer mutably when you call out to user code.

1 Like
3

Hi @lukasa, could explain more about this exclusive access violation crash? I would like to know the exact type and crash reason. Let's still use this exact RequestResponseHandler.swift, and say I only replace the errorCaught with my custom errCaught

    public func errorCaught(context: ChannelHandlerContext, error: Error) {
        guard self.state.isOperational else {
            assert(self.promiseBuffer.count == 0)
            return
        }
        self.state = .error(error)
        self.promiseBuffer.forEach { $0.fail(error) }
        self.promiseBuffer.removeAll()
        context.close(promise: nil)
        }
    }

In my understanding, if user is triggering a channel.write() on $0.fail(error) in self.promiseBuffer .forEach { $0.fail(error) }, and then leads to ChannelHandler.write() this will trigger a crash in my custom RequestResponseHandler?

Because you mentioned in my other question that, the channel handler methods are serialized as long as they are in only one Channel instance, so in my understanding, when you say

when you satisfy a promise, the user's callback code runs immediately

in this case, the user's channel.write() happens directly on the same thread right after $0.fail(error), so even the channel.write() is called, it's kind of being queued, and after my entire custom errorCaught has finished running, the ChannelHandler.write() starts to run then, but it would find the handler is in error state, and no more promises added into the buffer. So it looks like totally fine for me.

4

Sure, this is covered in the Swift book. The core sentence from that section is:

Specifically, a conflict occurs if you have two accesses that meet all of the following conditions:

  • At least one is a write access.
  • They access the same location in memory.
  • Their durations overlap.

In this case, self.promiseBuffer is the location in memory. During self.promiseBuffer.forEach you are accessing self.promiseBuffer for reading. The duration is the complete length of the call to forEach. Each of the calls to $0.fail(error) will call out to other code. If that code calls ChannelHandler.write, your write function will call self.promiseBuffer.append. This is another access to the same location in memory (self.promiseBuffer), it's a write access (append mutates), and it occurs during the duration of the previous read from forEach. This is a violation of Swift's exclusive access policy and will cause a crash.

That is not a correct understanding of what happens. Specifically, Channel.write has a fast-path that is invoked when the calling code is already running on the event loop thread. In that case, rather than enqueue the work it will simply immediately invoke it. This provides better performance, at the cost of leading to re-entrancy risks in your channel handler. You can see the fast-path in the code:

    public func write(_ data: NIOAny, promise: EventLoopPromise<Void>?) {
        if eventLoop.inEventLoop {
            write0(data, promise: promise)
        } else {
            eventLoop.execute {
                self.write0(data, promise: promise)
            }
        }
    }
2 Likes