Better backpressure management. (#352)

Motivation:

Users of the HTTPClientResponseDelegate expect that the event loop
futures returned from didReceiveHead and didReceiveBodyPart can be used
to exert backpressure. To be fair to them, they somewhat can. However,
the TaskHandler has a bit of a misunderstanding about how NIO
backpressure works, and does not correctly manage the buffer of inbound
data.

The result of this misunderstanding is that multiple calls to
didReceiveBodyPart and didReceiveHead can be outstanding at once. This
would likely lead to severe bugs in most delegates, as they do not
expect it.

We should make things work the way delegate implementers believe it
works.

Modifications:

- Added a buffer to the TaskHandler to avoid delivering data that the
   delegate is not ready for.
- Added a new "pending close" state that keeps track of a state where
   the TaskHandler has received .end but not yet delivered it to the
   delegate. This allows better error management.
- Added some more tests.
- Documented our backpressure commitments.

Result:

Better respect for backpressure.

Resolves #348
This commit is contained in:
Cory Benfield
2021-03-30 11:39:35 +01:00
committed by GitHub
parent b075d19007
commit e4fded76ac
6 changed files with 281 additions and 29 deletions
@@ -21,6 +21,7 @@ import NIOHTTP1
import NIOHTTPCompression
import NIOSSL
import NIOTransportServices
import XCTest
/// Are we testing NIO Transport services
func isTestingNIOTS() -> Bool {
@@ -100,6 +101,52 @@ class CountingDelegate: HTTPClientResponseDelegate {
}
}
class DelayOnHeadDelegate: HTTPClientResponseDelegate {
typealias Response = ByteBuffer
let promise: EventLoopPromise<Void>
private var data: ByteBuffer
private var mayReceiveData = false
private var expectError = false
init(promise: EventLoopPromise<Void>) {
self.promise = promise
self.data = ByteBuffer()
self.promise.futureResult.whenSuccess {
self.mayReceiveData = true
}
self.promise.futureResult.whenFailure { (_: Error) in
self.expectError = true
}
}
func didReceiveHead(task: HTTPClient.Task<Response>, _ head: HTTPResponseHead) -> EventLoopFuture<Void> {
XCTAssertFalse(self.expectError)
return self.promise.futureResult.hop(to: task.eventLoop)
}
func didReceiveBodyPart(task: HTTPClient.Task<Response>, _ buffer: ByteBuffer) -> EventLoopFuture<Void> {
XCTAssertTrue(self.mayReceiveData)
XCTAssertFalse(self.expectError)
self.data.writeImmutableBuffer(buffer)
return self.promise.futureResult.hop(to: task.eventLoop)
}
func didFinishRequest(task: HTTPClient.Task<Response>) throws -> Response {
XCTAssertTrue(self.mayReceiveData)
XCTAssertFalse(self.expectError)
return self.data
}
func didReceiveError(task: HTTPClient.Task<ByteBuffer>, _ error: Error) {
XCTAssertTrue(self.expectError)
}
}
internal final class RecordingHandler<Input, Output>: ChannelDuplexHandler {
typealias InboundIn = Input
typealias OutboundIn = Output
@@ -464,6 +511,21 @@ internal final class HttpBinHandler: ChannelInboundHandler {
context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil)
}
func writeChunked(context: ChannelHandlerContext) {
// This tests receiving chunks very fast: please do not insert delays here!
let headers = HTTPHeaders([("Transfer-Encoding", "chunked")])
context.write(self.wrapOutboundOut(.head(HTTPResponseHead(version: HTTPVersion(major: 1, minor: 1), status: .ok, headers: headers))), promise: nil)
for i in 0..<10 {
let msg = "id: \(i)"
var buf = context.channel.allocator.buffer(capacity: msg.count)
buf.writeString(msg)
context.write(wrapOutboundOut(.body(.byteBuffer(buf))), promise: nil)
}
context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil)
}
func channelRead(context: ChannelHandlerContext, data: NIOAny) {
self.isServingRequest = true
switch self.unwrapInboundIn(data) {
@@ -579,6 +641,19 @@ internal final class HttpBinHandler: ChannelInboundHandler {
return
case "/events/10/content-length":
self.writeEvents(context: context, isContentLengthRequired: true)
case "/chunked":
self.writeChunked(context: context)
return
case "/close-on-response":
var headers = self.responseHeaders
headers.replaceOrAdd(name: "connection", value: "close")
var builder = HTTPResponseBuilder(.http1_1, status: .ok, headers: headers)
builder.body = ByteBuffer(string: "some body content")
// We're forcing this closed now.
self.shouldClose = true
self.resps.append(builder)
default:
context.write(wrapOutboundOut(.head(HTTPResponseHead(version: HTTPVersion(major: 1, minor: 1), status: .notFound))), promise: nil)
context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil)