Skip to content

Commit 4fd1150

Browse files
dnadobadnadoba
authored
Fix bodyLengthMissmatch error handling (#490)
1 parent 6426c00 commit 4fd1150

File tree

3 files changed

+87
-41
lines changed

3 files changed

+87
-41
lines changed

‎Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift‎

Lines changed: 26 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -190,23 +190,42 @@ struct HTTPRequestStateMachine {
190190
}
191191

192192
mutating func errorHappened(_ error: Error) -> Action {
193+
if let error = error as? NIOSSLError,
194+
error == .uncleanShutdown,
195+
let action = self.handleNIOSSLUncleanShutdownError() {
196+
return action
197+
}
193198
switch self.state {
194199
case .initialized:
195200
preconditionFailure("After the state machine has been initialized, start must be called immediately. Thus this state is unreachable")
196201
case .waitForChannelToBecomeWritable:
197202
// the request failed, before it was sent onto the wire.
198203
self.state = .failed(error)
199204
return .failRequest(error, .none)
205+
case .running:
206+
self.state = .failed(error)
207+
return .failRequest(error, .close)
208+
209+
case .finished, .failed:
210+
// ignore error
211+
return .wait
200212

213+
case .modifying:
214+
preconditionFailure("Invalid state: \(self.state)")
215+
}
216+
}
217+
218+
private mutating func handleNIOSSLUncleanShutdownError() -> Action? {
219+
switch self.state {
201220
case .running(.streaming, .waitingForHead),
202-
.running(.endSent, .waitingForHead) where error as? NIOSSLError == .uncleanShutdown:
221+
.running(.endSent, .waitingForHead):
203222
// if we received a NIOSSL.uncleanShutdown before we got an answer we should handle
204223
// this like a normal connection close. We will receive a call to channelInactive after
205224
// this error.
206225
return .wait
207226

208227
case .running(.streaming, .receivingBody(let responseHead, _)),
209-
.running(.endSent, .receivingBody(let responseHead, _)) where error as? NIOSSLError == .uncleanShutdown:
228+
.running(.endSent, .receivingBody(let responseHead, _)):
210229
// This code is only reachable for request and responses, which we expect to have a body.
211230
// We depend on logic from the HTTPResponseDecoder here. The decoder will emit an
212231
// HTTPResponsePart.end right after the HTTPResponsePart.head, for every request with a
@@ -226,19 +245,11 @@ struct HTTPRequestStateMachine {
226245
// If the response is EOF terminated, we need to rely on a clean tls shutdown to be sure
227246
// we have received all necessary bytes. For this reason we forward the uncleanShutdown
228247
// error to the user.
229-
self.state = .failed(error)
230-
return .failRequest(error, .close)
231-
232-
case .running:
233-
self.state = .failed(error)
234-
return .failRequest(error, .close)
235-
236-
case .finished, .failed:
237-
// ignore error
238-
return .wait
248+
self.state = .failed(NIOSSLError.uncleanShutdown)
249+
return .failRequest(NIOSSLError.uncleanShutdown, .close)
239250

240-
case .modifying:
241-
preconditionFailure("Invalid state: \(self.state)")
251+
case .waitForChannelToBecomeWritable, .running, .finished, .failed, .initialized, .modifying:
252+
return nil
242253
}
243254
}
244255

@@ -270,7 +281,7 @@ struct HTTPRequestStateMachine {
270281

271282
if let expected = expectedBodyLength, sentBodyBytes + part.readableBytes > expected {
272283
let error = HTTPClientError.bodyLengthMismatch
273-
284+
self.state = .failed(error)
274285
return .failRequest(error, .close)
275286
}
276287

‎Tests/AsyncHTTPClientTests/HTTPRequestStateMachineTests+XCTest.swift‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,8 @@ extension HTTPRequestStateMachineTests {
5454
("testCanReadHTTP1_0ResponseWithBody", testCanReadHTTP1_0ResponseWithBody),
5555
("testFailHTTP1_0RequestThatIsStillUploading", testFailHTTP1_0RequestThatIsStillUploading),
5656
("testFailHTTP1RequestWithoutContentLengthWithNIOSSLErrorUncleanShutdown", testFailHTTP1RequestWithoutContentLengthWithNIOSSLErrorUncleanShutdown),
57+
("testNIOSSLErrorUncleanShutdownShouldBeTreatedAsRemoteConnectionCloseWhileInWaitingForHeadState", testNIOSSLErrorUncleanShutdownShouldBeTreatedAsRemoteConnectionCloseWhileInWaitingForHeadState),
58+
("testArbitraryErrorShouldBeTreatedAsARequestFailureWhileInWaitingForHeadState", testArbitraryErrorShouldBeTreatedAsARequestFailureWhileInWaitingForHeadState),
5759
("testFailHTTP1RequestWithContentLengthWithNIOSSLErrorUncleanShutdownButIgnoreIt", testFailHTTP1RequestWithContentLengthWithNIOSSLErrorUncleanShutdownButIgnoreIt),
5860
("testFailHTTPRequestWithContentLengthBecauseOfChannelInactiveWaitingForDemand", testFailHTTPRequestWithContentLengthBecauseOfChannelInactiveWaitingForDemand),
5961
("testFailHTTPRequestWithContentLengthBecauseOfChannelInactiveWaitingForRead", testFailHTTPRequestWithContentLengthBecauseOfChannelInactiveWaitingForRead),

‎Tests/AsyncHTTPClientTests/HTTPRequestStateMachineTests.swift‎

Lines changed: 59 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -76,12 +76,10 @@ class HTTPRequestStateMachineTests: XCTestCase {
7676
let part1 = IOData.byteBuffer(ByteBuffer(bytes: [0, 1, 2, 3]))
7777
XCTAssertEqual(state.requestStreamPartReceived(part0), .sendBodyPart(part0))
7878

79-
let failAction = state.requestStreamPartReceived(part1)
80-
guard case .failRequest(let error, .close) = failAction else {
81-
return XCTFail("Unexpected action: \(failAction)")
82-
}
79+
state.requestStreamPartReceived(part1).assertFailRequest(HTTPClientError.bodyLengthMismatch, .close)
8380

84-
XCTAssertEqual(error as? HTTPClientError, .bodyLengthMismatch)
81+
// if another error happens the new one is ignored
82+
XCTAssertEqual(state.errorHappened(HTTPClientError.remoteConnectionClosed), .wait)
8583
}
8684

8785
func testPOSTContentLengthIsTooShort() {
@@ -92,12 +90,7 @@ class HTTPRequestStateMachineTests: XCTestCase {
9290
let part0 = IOData.byteBuffer(ByteBuffer(bytes: [0, 1, 2, 3]))
9391
XCTAssertEqual(state.requestStreamPartReceived(part0), .sendBodyPart(part0))
9492

95-
let failAction = state.requestStreamFinished()
96-
guard case .failRequest(let error, .close) = failAction else {
97-
return XCTFail("Unexpected action: \(failAction)")
98-
}
99-
100-
XCTAssertEqual(error as? HTTPClientError, .bodyLengthMismatch)
93+
state.requestStreamFinished().assertFailRequest(HTTPClientError.bodyLengthMismatch, .close)
10194
}
10295

10396
func testRequestBodyStreamIsCancelledIfServerRespondsWith301() {
@@ -206,7 +199,7 @@ class HTTPRequestStateMachineTests: XCTestCase {
206199

207200
let part1 = IOData.byteBuffer(ByteBuffer(bytes: 4...7))
208201
XCTAssertEqual(state.requestStreamPartReceived(part1), .sendBodyPart(part1))
209-
XCTAssertEqual(state.requestStreamFinished(), .failRequest(HTTPClientError.bodyLengthMismatch, .close))
202+
state.requestStreamFinished().assertFailRequest(HTTPClientError.bodyLengthMismatch, .close)
210203
XCTAssertEqual(state.channelInactive(), .wait)
211204
}
212205

@@ -224,7 +217,7 @@ class HTTPRequestStateMachineTests: XCTestCase {
224217

225218
let part1 = IOData.byteBuffer(ByteBuffer(bytes: 4...7))
226219
XCTAssertEqual(state.requestStreamPartReceived(part1), .sendBodyPart(part1))
227-
XCTAssertEqual(state.requestStreamFinished(), .failRequest(HTTPClientError.bodyLengthMismatch, .close))
220+
state.requestStreamFinished().assertFailRequest(HTTPClientError.bodyLengthMismatch, .close)
228221
XCTAssertEqual(state.channelRead(.end(nil)), .wait)
229222
}
230223

@@ -249,7 +242,7 @@ class HTTPRequestStateMachineTests: XCTestCase {
249242
let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/")
250243
let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0))
251244
XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .wait)
252-
XCTAssertEqual(state.channelInactive(), .failRequest(HTTPClientError.remoteConnectionClosed, .none))
245+
state.channelInactive().assertFailRequest(HTTPClientError.remoteConnectionClosed, .none)
253246
}
254247

255248
func testResponseReadingWithBackpressure() {
@@ -339,15 +332,15 @@ class HTTPRequestStateMachineTests: XCTestCase {
339332

340333
func testCancellingARequestInStateInitializedKeepsTheConnectionAlive() {
341334
var state = HTTPRequestStateMachine(isChannelWritable: false)
342-
XCTAssertEqual(state.requestCancelled(), .failRequest(HTTPClientError.cancelled, .none))
335+
state.requestCancelled().assertFailRequest(HTTPClientError.cancelled, .none)
343336
}
344337

345338
func testCancellingARequestBeforeBeingSendKeepsTheConnectionAlive() {
346339
var state = HTTPRequestStateMachine(isChannelWritable: false)
347340
let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/")
348341
let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0))
349342
XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .wait)
350-
XCTAssertEqual(state.requestCancelled(), .failRequest(HTTPClientError.cancelled, .none))
343+
state.requestCancelled().assertFailRequest(HTTPClientError.cancelled, .none)
351344
}
352345

353346
func testConnectionBecomesWritableBeforeFirstRequest() {
@@ -373,15 +366,15 @@ class HTTPRequestStateMachineTests: XCTestCase {
373366
let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/")
374367
let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0))
375368
XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false))
376-
XCTAssertEqual(state.requestCancelled(), .failRequest(HTTPClientError.cancelled, .close))
369+
state.requestCancelled().assertFailRequest(HTTPClientError.cancelled, .close)
377370
}
378371

379372
func testRemoteSuddenlyClosesTheConnection() {
380373
var state = HTTPRequestStateMachine(isChannelWritable: true)
381374
let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/", headers: .init([("content-length", "4")]))
382375
let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(4))
383376
XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: true))
384-
XCTAssertEqual(state.requestCancelled(), .failRequest(HTTPClientError.remoteConnectionClosed, .close))
377+
state.requestCancelled().assertFailRequest(HTTPClientError.cancelled, .close)
385378
XCTAssertEqual(state.requestStreamPartReceived(.byteBuffer(.init(bytes: 1...3))), .wait)
386379
}
387380

@@ -395,7 +388,7 @@ class HTTPRequestStateMachineTests: XCTestCase {
395388
XCTAssertEqual(state.channelRead(.head(responseHead)), .forwardResponseHead(responseHead, pauseRequestBodyStream: false))
396389
let part0 = ByteBuffer(bytes: 0...3)
397390
XCTAssertEqual(state.channelRead(.body(part0)), .wait)
398-
XCTAssertEqual(state.idleReadTimeoutTriggered(), .failRequest(HTTPClientError.readTimeout, .close))
391+
state.idleReadTimeoutTriggered().assertFailRequest(HTTPClientError.readTimeout, .close)
399392
XCTAssertEqual(state.channelRead(.body(ByteBuffer(bytes: 4...7))), .wait)
400393
XCTAssertEqual(state.channelRead(.body(ByteBuffer(bytes: 8...11))), .wait)
401394
XCTAssertEqual(state.demandMoreResponseBodyParts(), .wait)
@@ -448,7 +441,7 @@ class HTTPRequestStateMachineTests: XCTestCase {
448441
let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0))
449442
XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false))
450443

451-
XCTAssertEqual(state.errorHappened(HTTPParserError.invalidChunkSize), .failRequest(HTTPParserError.invalidChunkSize, .close))
444+
state.errorHappened(HTTPParserError.invalidChunkSize).assertFailRequest(HTTPParserError.invalidChunkSize, .close)
452445
XCTAssertEqual(state.requestCancelled(), .wait, "A cancellation that happens to late is ignored")
453446
}
454447

@@ -502,7 +495,7 @@ class HTTPRequestStateMachineTests: XCTestCase {
502495
XCTAssertEqual(state.read(), .read)
503496
XCTAssertEqual(state.channelReadComplete(), .wait)
504497
XCTAssertEqual(state.channelRead(.body(body)), .wait)
505-
XCTAssertEqual(state.channelRead(.end(nil)), .failRequest(HTTPClientError.remoteConnectionClosed, .close))
498+
state.channelRead(.end(nil)).assertFailRequest(HTTPClientError.remoteConnectionClosed, .close)
506499
XCTAssertEqual(state.channelInactive(), .wait)
507500
}
508501

@@ -517,11 +510,32 @@ class HTTPRequestStateMachineTests: XCTestCase {
517510
XCTAssertEqual(state.channelRead(.head(responseHead)), .forwardResponseHead(responseHead, pauseRequestBodyStream: false))
518511
XCTAssertEqual(state.demandMoreResponseBodyParts(), .wait)
519512
XCTAssertEqual(state.channelRead(.body(body)), .wait)
520-
XCTAssertEqual(state.errorHappened(NIOSSLError.uncleanShutdown), .failRequest(NIOSSLError.uncleanShutdown, .close))
513+
state.errorHappened(NIOSSLError.uncleanShutdown).assertFailRequest(NIOSSLError.uncleanShutdown, .close)
521514
XCTAssertEqual(state.channelRead(.end(nil)), .wait)
522515
XCTAssertEqual(state.channelInactive(), .wait)
523516
}
524517

518+
func testNIOSSLErrorUncleanShutdownShouldBeTreatedAsRemoteConnectionCloseWhileInWaitingForHeadState() {
519+
var state = HTTPRequestStateMachine(isChannelWritable: true)
520+
let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/")
521+
let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0))
522+
XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false))
523+
524+
XCTAssertEqual(state.errorHappened(NIOSSLError.uncleanShutdown), .wait)
525+
state.channelInactive().assertFailRequest(HTTPClientError.remoteConnectionClosed, .none)
526+
}
527+
528+
func testArbitraryErrorShouldBeTreatedAsARequestFailureWhileInWaitingForHeadState() {
529+
struct ArbitraryError: Error, Equatable {}
530+
var state = HTTPRequestStateMachine(isChannelWritable: true)
531+
let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/")
532+
let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0))
533+
XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false))
534+
535+
state.errorHappened(ArbitraryError()).assertFailRequest(ArbitraryError(), .close)
536+
XCTAssertEqual(state.channelInactive(), .wait)
537+
}
538+
525539
func testFailHTTP1RequestWithContentLengthWithNIOSSLErrorUncleanShutdownButIgnoreIt() {
526540
var state = HTTPRequestStateMachine(isChannelWritable: true)
527541
let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/")
@@ -536,7 +550,7 @@ class HTTPRequestStateMachineTests: XCTestCase {
536550
XCTAssertEqual(state.channelRead(.body(body)), .wait)
537551
XCTAssertEqual(state.channelReadComplete(), .forwardResponseBodyParts([body]))
538552
XCTAssertEqual(state.errorHappened(NIOSSLError.uncleanShutdown), .wait)
539-
XCTAssertEqual(state.errorHappened(HTTPParserError.invalidEOFState), .failRequest(HTTPParserError.invalidEOFState, .close))
553+
state.errorHappened(HTTPParserError.invalidEOFState).assertFailRequest(HTTPParserError.invalidEOFState, .close)
540554
XCTAssertEqual(state.channelInactive(), .wait)
541555
}
542556

@@ -558,7 +572,7 @@ class HTTPRequestStateMachineTests: XCTestCase {
558572

559573
XCTAssertEqual(state.channelRead(.body(ByteBuffer(string: " baz lightyear"))), .wait)
560574
XCTAssertEqual(state.channelReadComplete(), .wait)
561-
XCTAssertEqual(state.channelInactive(), .failRequest(HTTPClientError.remoteConnectionClosed, .none))
575+
state.channelInactive().assertFailRequest(HTTPClientError.remoteConnectionClosed, .none)
562576
}
563577

564578
func testFailHTTPRequestWithContentLengthBecauseOfChannelInactiveWaitingForRead() {
@@ -579,7 +593,7 @@ class HTTPRequestStateMachineTests: XCTestCase {
579593

580594
XCTAssertEqual(state.channelRead(.body(ByteBuffer(string: " baz lightyear"))), .wait)
581595
XCTAssertEqual(state.channelReadComplete(), .wait)
582-
XCTAssertEqual(state.channelInactive(), .failRequest(HTTPClientError.remoteConnectionClosed, .none))
596+
state.channelInactive().assertFailRequest(HTTPClientError.remoteConnectionClosed, .none)
583597
}
584598

585599
func testFailHTTPRequestWithContentLengthBecauseOfChannelInactiveWaitingForReadAndDemand() {
@@ -599,7 +613,7 @@ class HTTPRequestStateMachineTests: XCTestCase {
599613

600614
XCTAssertEqual(state.channelRead(.body(ByteBuffer(string: " baz lightyear"))), .wait)
601615
XCTAssertEqual(state.channelReadComplete(), .wait)
602-
XCTAssertEqual(state.channelInactive(), .failRequest(HTTPClientError.remoteConnectionClosed, .none))
616+
state.channelInactive().assertFailRequest(HTTPClientError.remoteConnectionClosed, .none)
603617
}
604618

605619
func testFailHTTPRequestWithContentLengthBecauseOfChannelInactiveWaitingForReadAndDemandMultipleTimes() {
@@ -676,3 +690,22 @@ extension HTTPRequestStateMachine.Action: Equatable {
676690
}
677691
}
678692
}
693+
694+
extension HTTPRequestStateMachine.Action {
695+
fileprivate func assertFailRequest<Error>(
696+
_ expectedError: Error,
697+
_ expectedFinalStreamAction: HTTPRequestStateMachine.Action.FinalStreamAction,
698+
file: StaticString = #file,
699+
line: UInt = #line
700+
) where Error: Swift.Error & Equatable {
701+
guard case .failRequest(let actualError, let actualFinalStreamAction) = self else {
702+
return XCTFail("expected .failRequest(\(expectedError), \(expectedFinalStreamAction)) but got \(self)", file: file, line: line)
703+
}
704+
if let actualError = actualError as? Error {
705+
XCTAssertEqual(actualError, expectedError, file: file, line: line)
706+
} else {
707+
XCTFail("\(actualError) is not equal to \(expectedError)", file: file, line: line)
708+
}
709+
XCTAssertEqual(actualFinalStreamAction, expectedFinalStreamAction, file: file, line: line)
710+
}
711+
}

0 commit comments

Comments
 (0)