Skip to content

Commit 34ad4a6

Browse files
committed
review fixes
1 parent 3006123 commit 34ad4a6

File tree

4 files changed

+77
-39
lines changed

4 files changed

+77
-39
lines changed

Sources/AsyncHTTPClient/HTTPHandler.swift

Lines changed: 23 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -641,7 +641,7 @@ internal class TaskHandler<Delegate: HTTPClientResponseDelegate>: RemovableChann
641641
case head
642642
case redirected(HTTPResponseHead, URL)
643643
case body
644-
case end
644+
case endOrError
645645
}
646646

647647
let task: HTTPClient.Task<Delegate.Response>
@@ -782,7 +782,7 @@ extension TaskHandler: ChannelDuplexHandler {
782782
} catch {
783783
promise?.fail(error)
784784
self.failTaskAndNotifyDelegate(error: error, self.delegate.didReceiveError)
785-
self.state = .end
785+
self.state = .endOrError
786786
return
787787
}
788788

@@ -796,20 +796,24 @@ extension TaskHandler: ChannelDuplexHandler {
796796
assert(head.version == HTTPVersion(major: 1, minor: 1),
797797
"Sending a request in HTTP version \(head.version) which is unsupported by the above `if`")
798798

799-
self.expectedBodyLength = head.headers[canonicalForm: "content-length"].first.flatMap { Int($0) }
799+
800+
let contentLengths = head.headers[canonicalForm: "content-length"]
801+
assert(contentLengths.count <= 1)
802+
803+
self.expectedBodyLength = contentLengths.first.flatMap { Int($0) }
800804

801805
context.write(wrapOutboundOut(.head(head))).map {
802806
self.callOutToDelegateFireAndForget(value: head, self.delegate.didSendRequestHead)
803807
}.flatMap {
804808
self.writeBody(request: request, context: context)
805809
}.flatMap {
810+
context.eventLoop.assertInEventLoop()
806811
if let expectedBodyLength = self.expectedBodyLength, expectedBodyLength != self.actualBodyLength {
807-
self.state = .end
812+
self.state = .endOrError
808813
let error = HTTPClientError.bodyLengthMismatch
809814
self.failTaskAndNotifyDelegate(error: error, self.delegate.didReceiveError)
810815
return context.eventLoop.makeFailedFuture(error)
811816
}
812-
context.eventLoop.assertInEventLoop()
813817
return context.writeAndFlush(self.wrapOutboundOut(.end(nil)))
814818
}.map {
815819
context.eventLoop.assertInEventLoop()
@@ -818,10 +822,10 @@ extension TaskHandler: ChannelDuplexHandler {
818822
}.flatMapErrorThrowing { error in
819823
context.eventLoop.assertInEventLoop()
820824
switch self.state {
821-
case .end:
825+
case .endOrError:
822826
break
823827
default:
824-
self.state = .end
828+
self.state = .endOrError
825829
self.failTaskAndNotifyDelegate(error: error, self.delegate.didReceiveError)
826830
}
827831
throw error
@@ -839,14 +843,15 @@ extension TaskHandler: ChannelDuplexHandler {
839843
// All writes have to be switched to the channel EL if channel and task ELs differ
840844
if context.eventLoop.inEventLoop {
841845
context.writeAndFlush(self.wrapOutboundOut(.body(part)), promise: promise)
846+
self.actualBodyLength += part.readableBytes
842847
} else {
843848
context.eventLoop.execute {
844849
context.writeAndFlush(self.wrapOutboundOut(.body(part)), promise: promise)
850+
self.actualBodyLength += part.readableBytes
845851
}
846852
}
847853

848854
return promise.futureResult.map {
849-
self.actualBodyLength += part.readableBytes
850855
self.callOutToDelegateFireAndForget(value: part, self.delegate.didSendRequestPart)
851856
}
852857
})
@@ -904,12 +909,12 @@ extension TaskHandler: ChannelDuplexHandler {
904909
case .end:
905910
switch self.state {
906911
case .redirected(let head, let redirectURL):
907-
self.state = .end
912+
self.state = .endOrError
908913
self.task.releaseAssociatedConnection(delegateType: Delegate.self, closing: self.closing).whenSuccess {
909914
self.redirectHandler?.redirect(status: head.status, to: redirectURL, promise: self.task.promise)
910915
}
911916
default:
912-
self.state = .end
917+
self.state = .endOrError
913918
self.callOutToDelegate(promise: self.task.promise, self.delegate.didFinishRequest)
914919
}
915920
}
@@ -924,14 +929,14 @@ extension TaskHandler: ChannelDuplexHandler {
924929
context.read()
925930
}
926931
case .failure(let error):
927-
self.state = .end
932+
self.state = .endOrError
928933
self.failTaskAndNotifyDelegate(error: error, self.delegate.didReceiveError)
929934
}
930935
}
931936

932937
func userInboundEventTriggered(context: ChannelHandlerContext, event: Any) {
933938
if (event as? IdleStateHandler.IdleStateEvent) == .read {
934-
self.state = .end
939+
self.state = .endOrError
935940
let error = HTTPClientError.readTimeout
936941
self.failTaskAndNotifyDelegate(error: error, self.delegate.didReceiveError)
937942
} else {
@@ -941,7 +946,7 @@ extension TaskHandler: ChannelDuplexHandler {
941946

942947
func triggerUserOutboundEvent(context: ChannelHandlerContext, event: Any, promise: EventLoopPromise<Void>?) {
943948
if (event as? TaskCancelEvent) != nil {
944-
self.state = .end
949+
self.state = .endOrError
945950
let error = HTTPClientError.cancelled
946951
self.failTaskAndNotifyDelegate(error: error, self.delegate.didReceiveError)
947952
promise?.succeed(())
@@ -952,10 +957,10 @@ extension TaskHandler: ChannelDuplexHandler {
952957

953958
func channelInactive(context: ChannelHandlerContext) {
954959
switch self.state {
955-
case .end:
960+
case .endOrError:
956961
break
957962
case .body, .head, .idle, .redirected, .sent:
958-
self.state = .end
963+
self.state = .endOrError
959964
let error = HTTPClientError.remoteConnectionClosed
960965
self.failTaskAndNotifyDelegate(error: error, self.delegate.didReceiveError)
961966
}
@@ -966,7 +971,7 @@ extension TaskHandler: ChannelDuplexHandler {
966971
switch error {
967972
case NIOSSLError.uncleanShutdown:
968973
switch self.state {
969-
case .end:
974+
case .endOrError:
970975
/// Some HTTP Servers can 'forget' to respond with CloseNotify when client is closing connection,
971976
/// this could lead to incomplete SSL shutdown. But since request is already processed, we can ignore this error.
972977
break
@@ -975,11 +980,11 @@ extension TaskHandler: ChannelDuplexHandler {
975980
/// We can also ignore this error like `.end`.
976981
break
977982
default:
978-
self.state = .end
983+
self.state = .endOrError
979984
self.failTaskAndNotifyDelegate(error: error, self.delegate.didReceiveError)
980985
}
981986
default:
982-
self.state = .end
987+
self.state = .endOrError
983988
self.failTaskAndNotifyDelegate(error: error, self.delegate.didReceiveError)
984989
}
985990
}

Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,7 @@ internal final class HTTPBin {
188188
let group = MultiThreadedEventLoopGroup(numberOfThreads: 1)
189189
let serverChannel: Channel
190190
let isShutdown: NIOAtomic<Bool> = .makeAtomic(value: false)
191+
var connections: NIOAtomic<Int>
191192
var connectionCount: NIOAtomic<Int> = .makeAtomic(value: 0)
192193
private let activeConnCounterHandler: CountActiveConnectionsHandler
193194
var activeConnections: Int {
@@ -233,6 +234,9 @@ internal final class HTTPBin {
233234
let activeConnCounterHandler = CountActiveConnectionsHandler()
234235
self.activeConnCounterHandler = activeConnCounterHandler
235236

237+
let connections = NIOAtomic.makeAtomic(value: 0)
238+
self.connections = connections
239+
236240
self.serverChannel = try! ServerBootstrap(group: self.group)
237241
.serverChannelOption(ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEADDR), value: 1)
238242
.serverChannelInitializer { channel in
@@ -261,10 +265,10 @@ internal final class HTTPBin {
261265
}.flatMap {
262266
if ssl {
263267
return HTTPBin.configureTLS(channel: channel).flatMap {
264-
channel.pipeline.addHandler(HttpBinHandler(channelPromise: channelPromise, maxChannelAge: maxChannelAge))
268+
channel.pipeline.addHandler(HttpBinHandler(channelPromise: channelPromise, maxChannelAge: maxChannelAge, connectionId: connections.add(1)))
265269
}
266270
} else {
267-
return channel.pipeline.addHandler(HttpBinHandler(channelPromise: channelPromise, maxChannelAge: maxChannelAge))
271+
return channel.pipeline.addHandler(HttpBinHandler(channelPromise: channelPromise, maxChannelAge: maxChannelAge, connectionId: connections.add(1)))
268272
}
269273
}
270274
}
@@ -357,8 +361,8 @@ internal struct HTTPResponseBuilder {
357361
}
358362
}
359363

360-
let globalRequestCounter = NIOAtomic<Int>.makeAtomic(value: 0)
361-
let globalConnectionCounter = NIOAtomic<Int>.makeAtomic(value: 0)
364+
//let globalRequestCounter = NIOAtomic<Int>.makeAtomic(value: 0)
365+
//let globalConnectionCounter = NIOAtomic<Int>.makeAtomic(value: 0)
362366

363367
internal struct RequestInfo: Codable {
364368
var data: String
@@ -378,13 +382,13 @@ internal final class HttpBinHandler: ChannelInboundHandler {
378382
let maxChannelAge: TimeAmount?
379383
var shouldClose = false
380384
var isServingRequest = false
381-
let myConnectionNumber: Int
382-
var currentRequestNumber: Int = -1
385+
let connectionId: Int
386+
var requestId: Int = 0
383387

384-
init(channelPromise: EventLoopPromise<Channel>? = nil, maxChannelAge: TimeAmount? = nil) {
388+
init(channelPromise: EventLoopPromise<Channel>? = nil, maxChannelAge: TimeAmount? = nil, connectionId: Int) {
385389
self.channelPromise = channelPromise
386390
self.maxChannelAge = maxChannelAge
387-
self.myConnectionNumber = globalConnectionCounter.add(1)
391+
self.connectionId = connectionId
388392
}
389393

390394
func handlerAdded(context: ChannelHandlerContext) {
@@ -424,7 +428,7 @@ internal final class HttpBinHandler: ChannelInboundHandler {
424428
switch self.unwrapInboundIn(data) {
425429
case .head(let req):
426430
self.responseHeaders = HTTPHeaders()
427-
self.currentRequestNumber = globalRequestCounter.add(1)
431+
self.requestId += 1
428432
self.parseAndSetOptions(from: req)
429433
let urlComponents = URLComponents(string: req.uri)!
430434
switch urlComponents.percentEncodedPath {
@@ -552,8 +556,15 @@ internal final class HttpBinHandler: ChannelInboundHandler {
552556
context.write(wrapOutboundOut(.head(response.head)), promise: nil)
553557
if let body = response.body {
554558
let requestInfo = RequestInfo(data: String(buffer: body),
555-
requestNumber: self.currentRequestNumber,
556-
connectionNumber: self.myConnectionNumber)
559+
requestNumber: self.requestId,
560+
connectionNumber: self.connectionId)
561+
let responseBody = try! JSONEncoder().encodeAsByteBuffer(requestInfo,
562+
allocator: context.channel.allocator)
563+
context.write(wrapOutboundOut(.body(.byteBuffer(responseBody))), promise: nil)
564+
} else {
565+
let requestInfo = RequestInfo(data: "",
566+
requestNumber: self.requestId,
567+
connectionNumber: self.connectionId)
557568
let responseBody = try! JSONEncoder().encodeAsByteBuffer(requestInfo,
558569
allocator: context.channel.allocator)
559570
context.write(wrapOutboundOut(.body(.byteBuffer(responseBody))), promise: nil)

Tests/AsyncHTTPClientTests/HTTPClientTests.swift

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1713,7 +1713,8 @@ class HTTPClientTests: XCTestCase {
17131713

17141714
// req 1 and 2 cannot share the same connection (close header)
17151715
XCTAssertEqual(stats1.connectionNumber + 1, stats2.connectionNumber)
1716-
XCTAssertEqual(stats1.requestNumber + 1, stats2.requestNumber)
1716+
XCTAssertEqual(stats1.requestNumber, 1)
1717+
XCTAssertEqual(stats2.requestNumber, 1)
17171718

17181719
// req 2 and 3 should share the same connection (keep-alive is default)
17191720
XCTAssertEqual(stats2.requestNumber + 1, stats3.requestNumber)
@@ -1742,7 +1743,8 @@ class HTTPClientTests: XCTestCase {
17421743

17431744
// req 1 and 2 cannot share the same connection (close header)
17441745
XCTAssertEqual(stats1.connectionNumber + 1, stats2.connectionNumber)
1745-
XCTAssertEqual(stats1.requestNumber + 1, stats2.requestNumber)
1746+
XCTAssertEqual(stats1.requestNumber, 1)
1747+
XCTAssertEqual(stats2.requestNumber, 1)
17461748

17471749
// req 2 and 3 should share the same connection (keep-alive is default)
17481750
XCTAssertEqual(stats2.requestNumber + 1, stats3.requestNumber)
@@ -1773,7 +1775,7 @@ class HTTPClientTests: XCTestCase {
17731775

17741776
// req 1 and 2 cannot share the same connection (close header)
17751777
XCTAssertEqual(stats1.connectionNumber + 1, stats2.connectionNumber)
1776-
XCTAssertEqual(stats1.requestNumber + 1, stats2.requestNumber)
1778+
XCTAssertEqual(stats2.requestNumber, 1)
17771779

17781780
// req 2 and 3 should share the same connection (keep-alive is default)
17791781
XCTAssertEqual(stats2.requestNumber + 1, stats3.requestNumber)
@@ -1805,7 +1807,7 @@ class HTTPClientTests: XCTestCase {
18051807

18061808
// req 1 and 2 cannot share the same connection (close header)
18071809
XCTAssertEqual(stats1.connectionNumber + 1, stats2.connectionNumber)
1808-
XCTAssertEqual(stats1.requestNumber + 1, stats2.requestNumber)
1810+
XCTAssertEqual(stats2.requestNumber, 1)
18091811

18101812
// req 2 and 3 should share the same connection (keep-alive is default)
18111813
XCTAssertEqual(stats2.requestNumber + 1, stats3.requestNumber)
@@ -2052,22 +2054,29 @@ class HTTPClientTests: XCTestCase {
20522054
XCTAssertNoThrow(try future.wait())
20532055
}
20542056

2055-
func testContentLengthTooLongFails() {
2057+
func testContentLengthTooLongFails() throws {
20562058
let url = self.defaultHTTPBinURLPrefix + "/post"
20572059
XCTAssertThrowsError(
20582060
try self.defaultClient.execute(request:
20592061
Request(url: url,
20602062
body: .stream(length: 10) { streamWriter in
2061-
streamWriter.write(.byteBuffer(ByteBuffer(string: "1")))
2062-
})).wait()) { error in
2063+
let promise = self.defaultClient.eventLoopGroup.next().makePromise(of: Void.self)
2064+
DispatchQueue(label: "content-length-test").async {
2065+
streamWriter.write(.byteBuffer(ByteBuffer(string: "1"))).cascade(to: promise)
2066+
}
2067+
return promise.futureResult
2068+
})).wait()) { error in
20632069
XCTAssertEqual(error as! HTTPClientError, HTTPClientError.bodyLengthMismatch)
20642070
}
20652071
// Quickly try another request and check that it works.
2066-
XCTAssertNoThrow(try self.defaultClient.get(url: self.defaultHTTPBinURLPrefix + "/get").wait())
2072+
var response = try self.defaultClient.get(url: self.defaultHTTPBinURLPrefix + "get").wait()
2073+
let info = try response.body!.readJSONDecodable(RequestInfo.self, length: response.body!.readableBytes)
2074+
XCTAssertEqual(info!.connectionNumber, 1)
2075+
XCTAssertEqual(info!.requestNumber, 1)
20672076
}
20682077

20692078
// currently gets stuck because of #250 the server just never replies
2070-
func testContentLengthTooShortFails() {
2079+
func testContentLengthTooShortFails() throws {
20712080
let url = self.defaultHTTPBinURLPrefix + "/post"
20722081
let tooLong = "XBAD BAD BAD NOT HTTP/1.1\r\n\r\n"
20732082
XCTAssertThrowsError(
@@ -2080,6 +2089,9 @@ class HTTPClientTests: XCTestCase {
20802089
}
20812090
// Quickly try another request and check that it works. If we by accident wrote some extra bytes into the
20822091
// stream (and reuse the connection) that could cause problems.
2083-
XCTAssertNoThrow(try self.defaultClient.get(url: self.defaultHTTPBinURLPrefix + "/get").wait())
2092+
var response = try self.defaultClient.get(url: self.defaultHTTPBinURLPrefix + "get").wait()
2093+
let info = try response.body!.readJSONDecodable(RequestInfo.self, length: response.body!.readableBytes)
2094+
XCTAssertEqual(info!.connectionNumber, 1)
2095+
XCTAssertEqual(info!.requestNumber, 1)
20842096
}
20852097
}

Tests/AsyncHTTPClientTests/RequestValidationTests.swift

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,4 +104,14 @@ class RequestValidationTests: XCTestCase {
104104

105105
XCTAssertNoThrow(try headers.validate(method: .GET, body: nil))
106106
}
107+
108+
func testMultipleContentLengthOnNilStreamLength() {
109+
var headers = HTTPHeaders([("Content-Length", "1"), ("Content-Length", "2")])
110+
var buffer = ByteBufferAllocator().buffer(capacity: 10)
111+
buffer.writeBytes([UInt8](repeating: 12, count: 10))
112+
let body: HTTPClient.Body = .stream() { writer in
113+
writer.write(.byteBuffer(buffer))
114+
}
115+
XCTAssertThrowsError(try headers.validate(method: .PUT, body: body))
116+
}
107117
}

0 commit comments

Comments
 (0)