-
Notifications
You must be signed in to change notification settings - Fork 125
Ignore uncleanShutdown error when state is .head or .body #77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
38e1dc0
9bb04b9
657ca22
e8e0cf0
5ebe406
df2981c
ddc6385
8dd90c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -211,7 +211,7 @@ public class HTTPClient { | |
return channel.eventLoop.makeSucceededFuture(()) | ||
} | ||
}.flatMap { | ||
let taskHandler = TaskHandler(task: task, delegate: delegate, redirectHandler: redirectHandler) | ||
let taskHandler = TaskHandler(task: task, delegate: delegate, redirectHandler: redirectHandler, ignoreNIOSSLUncleanShutdownError: self.configuration.ignoreNIOSSLUncleanShutdownError) | ||
return channel.pipeline.addHandler(taskHandler) | ||
} | ||
} | ||
|
@@ -276,19 +276,31 @@ public class HTTPClient { | |
public var timeout: Timeout | ||
/// Upstream proxy, defaults to no proxy. | ||
public var proxy: Proxy? | ||
/// Ignore TLS unclean shutdown error, defaults to `false`. | ||
public var ignoreNIOSSLUncleanShutdownError: Bool | ||
|
||
public init(tlsConfiguration: TLSConfiguration? = nil, followRedirects: Bool = false, timeout: Timeout = Timeout(), proxy: Proxy? = nil) { | ||
self.init(tlsConfiguration: tlsConfiguration, followRedirects: followRedirects, timeout: timeout, proxy: proxy, ignoreNIOSSLUncleanShutdownError: false) | ||
} | ||
|
||
public init(tlsConfiguration: TLSConfiguration? = nil, followRedirects: Bool = false, timeout: Timeout = Timeout(), proxy: Proxy? = nil, ignoreNIOSSLUncleanShutdownError: Bool = false) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change needs to be done differently, I'm afraid. This is a breaking API change, as you are allowed to refer to functions in Swift by their complete set of labels, and this changes the set of labels here. You need to do this: public init(tlsConfiguration: TLSConfiguration? = nil, followRedirects: Bool = false, timeout: Timeout = Timeout(), proxy: Proxy? = nil) {
self.init(tlsConfiguration: tlsConfiguration, followRedirects: followRedirects, timeout: timeout, proxy: proxy, ignoreNIOSSLUncleanShutdownError: true)
}
public init(tlsConfiguration: TLSConfiguration? = nil, followRedirects: Bool = false, timeout: Timeout = Timeout(), proxy: Proxy? = nil, ignoreNIOSSLUncleanShutdownError: Bool) {
self.tlsConfiguration = tlsConfiguration
self.followRedirects = followRedirects
self.timeout = timeout
self.proxy = proxy
self.ignoreNIOSSLUncleanShutdownError = ignoreNIOSSLUncleanShutdownError
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was reverted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Lukasa we are not tagged There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, if we're not maintaining a stable API we can do that. |
||
self.tlsConfiguration = tlsConfiguration | ||
self.followRedirects = followRedirects | ||
self.timeout = timeout | ||
self.proxy = proxy | ||
self.ignoreNIOSSLUncleanShutdownError = ignoreNIOSSLUncleanShutdownError | ||
} | ||
|
||
public init(certificateVerification: CertificateVerification, followRedirects: Bool = false, timeout: Timeout = Timeout(), proxy: Proxy? = nil) { | ||
self.init(certificateVerification: certificateVerification, followRedirects: followRedirects, timeout: timeout, proxy: proxy, ignoreNIOSSLUncleanShutdownError: false) | ||
} | ||
|
||
public init(certificateVerification: CertificateVerification, followRedirects: Bool = false, timeout: Timeout = Timeout(), proxy: Proxy? = nil, ignoreNIOSSLUncleanShutdownError: Bool = false) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This requires a similar change as the one above: we need two functions, not one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was reverted too. |
||
self.tlsConfiguration = TLSConfiguration.forClient(certificateVerification: certificateVerification) | ||
self.followRedirects = followRedirects | ||
self.timeout = timeout | ||
self.proxy = proxy | ||
self.ignoreNIOSSLUncleanShutdownError = ignoreNIOSSLUncleanShutdownError | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ import AsyncHTTPClient | |
import NIO | ||
import NIOFoundationCompat | ||
import NIOHTTP1 | ||
import NIOSSL | ||
import XCTest | ||
|
||
class HTTPClientTests: XCTestCase { | ||
|
@@ -346,4 +347,141 @@ class HTTPClientTests: XCTestCase { | |
XCTAssertEqual(.ok, response.status) | ||
XCTAssertEqual("12344321", data.data) | ||
} | ||
|
||
func testNoContentLengthForSSLUncleanShutdown() throws { | ||
let httpBin = HttpBinForSSLUncleanShutdown() | ||
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew, | ||
configuration: HTTPClient.Configuration(certificateVerification: .none)) | ||
|
||
defer { | ||
try! httpClient.syncShutdown() | ||
httpBin.shutdown() | ||
} | ||
|
||
XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(httpBin.port)/nocontentlength").wait(), "Should fail") { error in | ||
guard case let error = error as? NIOSSLError, case .uncleanShutdown = error else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
return XCTFail("Should fail with NIOSSLError.uncleanShutdown") | ||
} | ||
} | ||
} | ||
|
||
func testNoContentLengthWithIgnoreErrorForSSLUncleanShutdown() throws { | ||
let httpBin = HttpBinForSSLUncleanShutdown() | ||
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew, | ||
configuration: HTTPClient.Configuration(certificateVerification: .none, ignoreNIOSSLUncleanShutdownError: true)) | ||
|
||
defer { | ||
try! httpClient.syncShutdown() | ||
httpBin.shutdown() | ||
} | ||
|
||
let response = try httpClient.get(url: "https://localhost:\(httpBin.port)/nocontentlength").wait() | ||
let bytes = response.body.flatMap { $0.getData(at: 0, length: $0.readableBytes) } | ||
let string = String(decoding: bytes!, as: UTF8.self) | ||
|
||
XCTAssertEqual(.ok, response.status) | ||
XCTAssertEqual("foo", string) | ||
} | ||
|
||
func testCorrectContentLengthForSSLUncleanShutdown() throws { | ||
let httpBin = HttpBinForSSLUncleanShutdown() | ||
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew, | ||
configuration: HTTPClient.Configuration(certificateVerification: .none)) | ||
|
||
defer { | ||
try! httpClient.syncShutdown() | ||
httpBin.shutdown() | ||
} | ||
|
||
let response = try httpClient.get(url: "https://localhost:\(httpBin.port)/").wait() | ||
let bytes = response.body.flatMap { $0.getData(at: 0, length: $0.readableBytes) } | ||
let string = String(decoding: bytes!, as: UTF8.self) | ||
|
||
XCTAssertEqual(.notFound, response.status) | ||
XCTAssertEqual("Not Found", string) | ||
} | ||
|
||
func testNoContentForSSLUncleanShutdown() throws { | ||
let httpBin = HttpBinForSSLUncleanShutdown() | ||
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew, | ||
configuration: HTTPClient.Configuration(certificateVerification: .none)) | ||
|
||
defer { | ||
try! httpClient.syncShutdown() | ||
httpBin.shutdown() | ||
} | ||
|
||
let response = try httpClient.get(url: "https://localhost:\(httpBin.port)/nocontent").wait() | ||
|
||
XCTAssertEqual(.noContent, response.status) | ||
XCTAssertEqual(response.body, nil) | ||
} | ||
|
||
func testNoResponseForSSLUncleanShutdown() throws { | ||
let httpBin = HttpBinForSSLUncleanShutdown() | ||
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew, | ||
configuration: HTTPClient.Configuration(certificateVerification: .none)) | ||
|
||
defer { | ||
try! httpClient.syncShutdown() | ||
httpBin.shutdown() | ||
} | ||
|
||
XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(httpBin.port)/noresponse").wait(), "Should fail") { error in | ||
guard case let error = error as? NIOSSLError, case .uncleanShutdown = error else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
return XCTFail("Should fail with NIOSSLError.uncleanShutdown") | ||
} | ||
} | ||
} | ||
|
||
func testNoResponseWithIgnoreErrorForSSLUncleanShutdown() throws { | ||
let httpBin = HttpBinForSSLUncleanShutdown() | ||
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew, | ||
configuration: HTTPClient.Configuration(certificateVerification: .none, ignoreNIOSSLUncleanShutdownError: true)) | ||
|
||
defer { | ||
try! httpClient.syncShutdown() | ||
httpBin.shutdown() | ||
} | ||
|
||
XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(httpBin.port)/noresponse").wait(), "Should fail") { error in | ||
guard case let error = error as? NIOSSLError, case .uncleanShutdown = error else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
return XCTFail("Should fail with NIOSSLError.uncleanShutdown") | ||
} | ||
} | ||
} | ||
|
||
func testWrongContentLengthForSSLUncleanShutdown() throws { | ||
let httpBin = HttpBinForSSLUncleanShutdown() | ||
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew, | ||
configuration: HTTPClient.Configuration(certificateVerification: .none)) | ||
|
||
defer { | ||
try! httpClient.syncShutdown() | ||
httpBin.shutdown() | ||
} | ||
|
||
XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(httpBin.port)/wrongcontentlength").wait(), "Should fail") { error in | ||
guard case let error = error as? NIOSSLError, case .uncleanShutdown = error else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
return XCTFail("Should fail with NIOSSLError.uncleanShutdown") | ||
} | ||
} | ||
} | ||
|
||
func testWrongContentLengthWithIgnoreErrorForSSLUncleanShutdown() throws { | ||
let httpBin = HttpBinForSSLUncleanShutdown() | ||
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew, | ||
configuration: HTTPClient.Configuration(certificateVerification: .none, ignoreNIOSSLUncleanShutdownError: true)) | ||
|
||
defer { | ||
try! httpClient.syncShutdown() | ||
httpBin.shutdown() | ||
} | ||
|
||
XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(httpBin.port)/wrongcontentlength").wait(), "Should fail") { error in | ||
guard case let error = error as? HTTPParserError, case .invalidEOFState = error else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, it's my mistakes. it was updated, and I have run tests in swift 5.0 . |
||
return XCTFail("Should fail with HTTPParserError.invalidEOFState") | ||
} | ||
} | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=>
ignoreNIOSSLUncleanShutdownError
is a mouthful imo, maybeignoreUncleanSSLShutdown
? or something even shorter if someone can come up with a suggestion?=> what are the arguments for the default to be false vs. true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m in favour of a shorter name.
False is a good default because we already ignore it in the cases where it’s 100% safe to do so: setting this switch to true would ignore it in the cases where it potentially opens your application up to some theoretical risks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe
ignoreSSLUncleanShutdown
?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignoreUncleanSSLShutdown
seems more natural? subjective obviously, so up to @artemredkinThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for
ignoreUncleanSSLShutdown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomerd It was renamed.