ResponseDelegate can invoke `didReceiveBodyPart` before the previous run's future returned

[adam-fowler]

I've added a PR #273 to verify this

[weissi]

Yes, right now, the response delegate may be called multiple times. The backpressure itself does seem to work (it won't give you arbitrary amounts of data until you processed it) but there's no guarantee it'll only be invoked once.

The code backs this up

    func channelRead(context: ChannelHandlerContext, data: NIOAny) {
        let response = self.unwrapInboundIn(data)
        switch response {
        case .head(let head):
            [...]
        case .body(let body):
            switch self.state {
            case .redirected:
                break
            default:
                self.state = .body
                self.mayRead = false
                self.callOutToDelegate(value: body, channelEventLoop: context.eventLoop, self.delegate.didReceiveBodyPart)
                    .whenComplete { result in
                        self.handleBackpressureResult(context: context, result: result)
                    }
            }
        case .end:
            [...]
    }


    public func read(context: ChannelHandlerContext) {
        if self.mayRead {
            self.pendingRead = false
            context.read()
        } else {
            self.pendingRead = true
        }
    }

As we can see, AHC will send everything to the delegate straight away that comes through channelRead. channelRead however can be invoked multiple times even if you're not reading. For example because:

• a ChannelHandler in front just invokes it multiple times
• we receive EOF (when NIO will drain the socket buffer and deliver the remaining data)
• maxMessagesPerRead != 1

AHC is meant for end users so I would agree that it should invoke the delegate again only after having seen the previous future succeed. That's by far the most useful model for users who may say want to write the received body to disk or so.

[SwiftNIO itself couldn't reasonably make such a guarantee because the user (ie the ChannelHandlers are in charge of calling channelRead). Sure, NIO could in theory buffer before/after each ChannelHandler but that would add a lot of memory bloat that the user can't control.]

[weissi]

CC @artemredkin . Should we make this a 1.2.0 release blocker or are we "happy" with it given that it was always broken?

[artemredkin]

I think we should schedule it for 1.2.1 milestone since it was already broken.

[adam-fowler]

The test in #273 also shows that the HTTPClient finishes before all the ResponseDelegate tasks are finished. Would this be considered a different bug?

[adam-fowler]

The test in #273 also shows that the HTTPClient finishes before all the ResponseDelegate tasks are finished. Would this be considered a different bug?

This would be fixed by fixing the above bug, but this could possibly be resolved without fixing the above. I believe it is a more serious issue than the original bug filed. Should this get its own issue? It has been sitting around for 6 months now.