From c22e5ead6bcf464f96ec3d1ec95886b0cde49357 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Thu, 2 Nov 2017 12:56:00 +0100 Subject: [PATCH] Remove ByteBufferAllocator constructor argument from HttpResponseEncoder Motivation: We can allocate the buffer on the fly if needed, no need to add the constructor argument. Modifications: Just allocate the buffer in the methods itself that need it. Result: Easier to construct encoder and cleaner code. --- Sources/NIOHTTP1/HTTP.swift | 55 ++++++++++--------- Sources/NIOHTTP1Server/main.swift | 2 +- Tests/NIOHTTP1Tests/HTTPEncoderTest.swift | 4 +- .../NIOHTTP1Tests/HTTPServerClientTest.swift | 8 +-- Tests/NIOHTTP1Tests/HTTPUpgradeTests.swift | 2 +- 5 files changed, 36 insertions(+), 35 deletions(-) diff --git a/Sources/NIOHTTP1/HTTP.swift b/Sources/NIOHTTP1/HTTP.swift index 987d5e0b..c790fa32 100644 --- a/Sources/NIOHTTP1/HTTP.swift +++ b/Sources/NIOHTTP1/HTTP.swift @@ -20,11 +20,8 @@ public final class HTTPResponseEncoder : ChannelOutboundHandler { public typealias OutboundOut = IOData private var isChunked = false - private var scratchBuffer: ByteBuffer - public init(allocator: ByteBufferAllocator = ByteBufferAllocator()) { - self.scratchBuffer = allocator.buffer(capacity: 256) - } + public init () { } private func writeChunk(ctx: ChannelHandlerContext, chunk: IOData, promise: Promise?) { let (mW1, mW2, mW3): (Promise<()>?, Promise<()>?, Promise<()>?) @@ -47,19 +44,20 @@ public final class HTTPResponseEncoder : ChannelOutboundHandler { /* we don't want to copy the chunk unnecessarily and therefore call write an annoyingly large number of times */ if self.isChunked { - self.scratchBuffer.clear() + var buffer = ctx.channel!.allocator.buffer(capacity: 32) let len = String(readableBytes, radix: 16) - self.scratchBuffer.write(string: len) - self.scratchBuffer.write(staticString: "\r\n") - ctx.write(data: self.wrapOutboundOut(.byteBuffer(self.scratchBuffer)), promise: mW1) - } - - ctx.write(data: self.wrapOutboundOut(chunk), promise: mW2) + buffer.write(string: len) + buffer.write(staticString: "\r\n") + ctx.write(data: self.wrapOutboundOut(.byteBuffer(buffer)), promise: mW1) - if self.isChunked { - self.scratchBuffer.clear() - self.scratchBuffer.write(staticString: "\r\n") - ctx.write(data: self.wrapOutboundOut(.byteBuffer(self.scratchBuffer)), promise: mW3) + ctx.write(data: self.wrapOutboundOut(chunk), promise: mW2) + + // Just clear the buffer as we depend on the COW semantics. + buffer.clear() + buffer.write(staticString: "\r\n") + ctx.write(data: self.wrapOutboundOut(.byteBuffer(buffer)), promise: mW3) + } else { + ctx.write(data: self.wrapOutboundOut(chunk), promise: mW2) } } @@ -69,27 +67,30 @@ public final class HTTPResponseEncoder : ChannelOutboundHandler { sanitizeTransportHeaders(status: response.status, headers: &response.headers, version: response.version) self.isChunked = response.headers["transfer-encoding"].contains("chunked") - self.scratchBuffer.clear() - response.version.write(buffer: &self.scratchBuffer) - self.scratchBuffer.write(staticString: " ") - response.status.write(buffer: &self.scratchBuffer) - self.scratchBuffer.write(staticString: "\r\n") - response.headers.write(buffer: &self.scratchBuffer) + var buffer = ctx.channel!.allocator.buffer(capacity: 256) - ctx.write(data: self.wrapOutboundOut(.byteBuffer(self.scratchBuffer)), promise: promise) + response.version.write(buffer: &buffer) + buffer.write(staticString: " ") + response.status.write(buffer: &buffer) + buffer.write(staticString: "\r\n") + response.headers.write(buffer: &buffer) + + ctx.write(data: self.wrapOutboundOut(.byteBuffer(buffer)), promise: promise) case .some(.body(let bodyPart)): self.writeChunk(ctx: ctx, chunk: bodyPart, promise: promise) case .some(.end(let trailers)): switch (self.isChunked, promise) { case (true, let p): - self.scratchBuffer.clear() + var buffer: ByteBuffer if let trailers = trailers { - self.scratchBuffer.write(staticString: "0\r\n") - trailers.write(buffer: &self.scratchBuffer) // Includes trailing CRLF. + buffer = ctx.channel!.allocator.buffer(capacity: 256) + buffer.write(staticString: "0\r\n") + trailers.write(buffer: &buffer) // Includes trailing CRLF. } else { - self.scratchBuffer.write(staticString: "0\r\n\r\n") + buffer = ctx.channel!.allocator.buffer(capacity: 8) + buffer.write(staticString: "0\r\n\r\n") } - ctx.write(data: self.wrapOutboundOut(.byteBuffer(self.scratchBuffer)), promise: p) + ctx.write(data: self.wrapOutboundOut(.byteBuffer(buffer)), promise: p) case (false, .some(let p)): // Not chunked so we have nothing to write. However, we don't want to satisfy this promise out-of-order // so we issue a zero-length write down the chain. diff --git a/Sources/NIOHTTP1Server/main.swift b/Sources/NIOHTTP1Server/main.swift index f1ae6418..fb2f778f 100644 --- a/Sources/NIOHTTP1Server/main.swift +++ b/Sources/NIOHTTP1Server/main.swift @@ -76,7 +76,7 @@ let bootstrap = ServerBootstrap(group: group) // Set the handlers that are applied to the accepted Channels .handler(childHandler: ChannelInitializer(initChannel: { channel in - return channel.pipeline.add(handler: HTTPResponseEncoder(allocator: channel.allocator)).then(callback: { v2 in + return channel.pipeline.add(handler: HTTPResponseEncoder()).then(callback: { v2 in return channel.pipeline.add(handler: HTTPRequestDecoder()).then(callback: { v2 in return channel.pipeline.add(handler: HTTPHandler()) }) diff --git a/Tests/NIOHTTP1Tests/HTTPEncoderTest.swift b/Tests/NIOHTTP1Tests/HTTPEncoderTest.swift index 507162bb..7f30d227 100644 --- a/Tests/NIOHTTP1Tests/HTTPEncoderTest.swift +++ b/Tests/NIOHTTP1Tests/HTTPEncoderTest.swift @@ -29,7 +29,7 @@ class HTTPEncoderTests: XCTestCase { XCTAssertFalse(try! channel.finish()) } - try! channel.pipeline.add(handler: HTTPResponseEncoder(allocator: channel.allocator)).wait() + try! channel.pipeline.add(handler: HTTPResponseEncoder()).wait() var switchingResponse = HTTPResponseHead(version: HTTPVersion(major: 1, minor:1), status: status) switchingResponse.headers = headers try! channel.writeOutbound(data: HTTPResponsePart.head(switchingResponse)) @@ -98,7 +98,7 @@ class HTTPEncoderTests: XCTestCase { XCTAssertFalse(try! channel.finish()) } - try! channel.pipeline.add(handler: HTTPResponseEncoder(allocator: channel.allocator)).wait() + try! channel.pipeline.add(handler: HTTPResponseEncoder()).wait() // This response contains neither Transfer-Encoding: chunked or Content-Length. let response = HTTPResponseHead(version: HTTPVersion(major: 1, minor:0), status: .ok) diff --git a/Tests/NIOHTTP1Tests/HTTPServerClientTest.swift b/Tests/NIOHTTP1Tests/HTTPServerClientTest.swift index b78e3b18..9c9eb29a 100644 --- a/Tests/NIOHTTP1Tests/HTTPServerClientTest.swift +++ b/Tests/NIOHTTP1Tests/HTTPServerClientTest.swift @@ -277,7 +277,7 @@ class HTTPServerClientTest : XCTestCase { .handler(childHandler: ChannelInitializer(initChannel: { channel in // Ensure we not read faster then we can write by adding the BackPressureHandler into the pipeline. channel.pipeline.add(handler: HTTPRequestDecoder()).then { - channel.pipeline.add(handler: HTTPResponseEncoder(allocator: channel.allocator)).then { + channel.pipeline.add(handler: HTTPResponseEncoder()).then { channel.pipeline.add(handler: httpHandler) } } @@ -334,7 +334,7 @@ class HTTPServerClientTest : XCTestCase { .handler(childHandler: ChannelInitializer(initChannel: { channel in // Ensure we not read faster then we can write by adding the BackPressureHandler into the pipeline. channel.pipeline.add(handler: HTTPRequestDecoder()).then { - channel.pipeline.add(handler: HTTPResponseEncoder(allocator: channel.allocator)).then { + channel.pipeline.add(handler: HTTPResponseEncoder()).then { channel.pipeline.add(handler: httpHandler) } } @@ -392,7 +392,7 @@ class HTTPServerClientTest : XCTestCase { .option(option: ChannelOptions.Socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEADDR), value: 1) .handler(childHandler: ChannelInitializer(initChannel: { channel in channel.pipeline.add(handler: HTTPRequestDecoder()).then { - channel.pipeline.add(handler: HTTPResponseEncoder(allocator: channel.allocator)).then { + channel.pipeline.add(handler: HTTPResponseEncoder()).then { channel.pipeline.add(handler: httpHandler) } } @@ -449,7 +449,7 @@ class HTTPServerClientTest : XCTestCase { .handler(childHandler: ChannelInitializer(initChannel: { channel in // Ensure we not read faster then we can write by adding the BackPressureHandler into the pipeline. channel.pipeline.add(handler: HTTPRequestDecoder()).then { - channel.pipeline.add(handler: HTTPResponseEncoder(allocator: channel.allocator)).then { + channel.pipeline.add(handler: HTTPResponseEncoder()).then { channel.pipeline.add(handler: httpHandler) } } diff --git a/Tests/NIOHTTP1Tests/HTTPUpgradeTests.swift b/Tests/NIOHTTP1Tests/HTTPUpgradeTests.swift index 4442046f..8c924e9f 100644 --- a/Tests/NIOHTTP1Tests/HTTPUpgradeTests.swift +++ b/Tests/NIOHTTP1Tests/HTTPUpgradeTests.swift @@ -32,7 +32,7 @@ private func serverHTTPChannel(group: EventLoopGroup, handlers: [ChannelHandler] .option(option: ChannelOptions.Socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEADDR), value: 1) .handler(childHandler: ChannelInitializer(initChannel: { channel in channel.pipeline.add(handler: HTTPRequestDecoder()).then { - channel.pipeline.add(handler: HTTPResponseEncoder(allocator: channel.allocator)).then { + channel.pipeline.add(handler: HTTPResponseEncoder()).then { let futureResults = handlers.map { channel.pipeline.add(handler: $0) } return Future.andAll(futureResults, eventLoop: channel.eventLoop) }