From 2383b65fce3291608fa8d888025b885496527959 Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Sat, 13 Jun 2020 20:19:39 +0100 Subject: [PATCH 01/12] assume chunked on zero-length stream --- .../AsyncHTTPClient/RequestValidation.swift | 12 ++++--- .../HTTPClientTests.swift | 31 +++++++++++++++++++ 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/Sources/AsyncHTTPClient/RequestValidation.swift b/Sources/AsyncHTTPClient/RequestValidation.swift index e5061a99a..c01229015 100644 --- a/Sources/AsyncHTTPClient/RequestValidation.swift +++ b/Sources/AsyncHTTPClient/RequestValidation.swift @@ -27,11 +27,11 @@ extension HTTPHeaders { } self.remove(name: "Transfer-Encoding") - self.remove(name: "Content-Length") try self.validateFieldNames() guard let body = body else { + self.remove(name: "Content-Length") // if we don't have a body we might not need to send the Content-Length field // https://tools.ietf.org/html/rfc7230#section-3.3.2 switch method { @@ -60,11 +60,15 @@ extension HTTPHeaders { } if encodings.isEmpty { - guard let length = body.length else { - throw HTTPClientError.contentLengthMissing + if let length = body.length { + self.remove(name: "Content-Length") + contentLength = length + } else if !self.contains(name: "Content-Length") { + transferEncoding = "chunked" } - contentLength = length } else { + self.remove(name: "Content-Length") + transferEncoding = encodings.joined(separator: ", ") if !encodings.contains("chunked") { guard let length = body.length else { diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 0e4e25d6e..fed42ae83 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2005,4 +2005,35 @@ class HTTPClientTests: XCTestCase { self.defaultClient = nil // so it doesn't get shut down again. } + + func testUploadStreamingNoLength() throws { + let server = NIOHTTP1TestServer(group: self.serverGroup) + let client = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup)) + defer { + XCTAssertNoThrow(try client.syncShutdown()) + XCTAssertNoThrow(try server.stop()) + } + + var request = try HTTPClient.Request(url: "http://localhost:\(server.serverPort)/") + request.body = .stream() { writer in + writer.write(.byteBuffer(ByteBuffer.of(string: "1234"))) + } + + let future = client.execute(request: request) + + switch try server.readInbound() { + case .head(let head): + XCTAssertEqual(head.headers["transfer-encoding"], ["chunked"]) + default: + XCTFail("Unexpected part") + } + + XCTAssertNoThrow(try server.readInbound()) // .body + XCTAssertNoThrow(try server.readInbound()) // .end + + XCTAssertNoThrow(try server.writeOutbound(.head(.init(version: .init(major: 1, minor: 1), status: .ok)))) + XCTAssertNoThrow(try server.writeOutbound(.end(nil))) + + XCTAssertNoThrow(try future.wait()) + } } From 084dee8c3c785b7821cd96a46bc60b92ed649fdf Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Sat, 13 Jun 2020 20:24:11 +0100 Subject: [PATCH 02/12] linux test and swift format --- Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift | 1 + Tests/AsyncHTTPClientTests/HTTPClientTests.swift | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 7d61f805e..362cf9496 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -109,6 +109,7 @@ extension HTTPClientTests { ("testNothingIsLoggedAtInfoOrHigher", testNothingIsLoggedAtInfoOrHigher), ("testAllMethodsLog", testAllMethodsLog), ("testClosingIdleConnectionsInPoolLogsInTheBackground", testClosingIdleConnectionsInPoolLogsInTheBackground), + ("testUploadStreamingNoLength", testUploadStreamingNoLength), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index fed42ae83..228f6e551 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2015,7 +2015,7 @@ class HTTPClientTests: XCTestCase { } var request = try HTTPClient.Request(url: "http://localhost:\(server.serverPort)/") - request.body = .stream() { writer in + request.body = .stream { writer in writer.write(.byteBuffer(ByteBuffer.of(string: "1234"))) } From 995a98b0c2b0e1f00c8e62593e63816897403d0c Mon Sep 17 00:00:00 2001 From: Dimitri Bouniol Date: Fri, 19 Jun 2020 02:51:03 -0700 Subject: [PATCH 03/12] Convenience methods for socket paths (#235) * Added additional tests for socketPath-based requests Motivation: While going through the existing tests, I identified a few more instances where we could add some testing. Modifications: Added one test that verifies Requests are being decoded correctly, and improved three others to check for path parsing, error throwing, and schema casing respectively. Result: Tests that continue to pass, but that will also catch any incompatible changes in the future. * Added some convenience initializers to URL and methods to Request for making requests to socket paths Motivation: Creating URLs for connecting to servers bound to socket paths currently requires some additional code to get exactly right. It would be nice to have convenience methods on both URL and Request to assist here. Modifications: - Refactored the get/post/patch/put/delete methods so they all call into a one line execute() method. - Added variations on the above methods so they can be called with socket paths (both over HTTP and HTTPS). - Added public convenience initializers to URL to support the above, and so socket path URLs can be easily created in other situations. - Added unit tests for creating socket path URLs, and testing the new suite of convenience execute methods (that, er, test `HTTPMETHOD`s). (patch, put, and delete are now also tested as a result of these tests) - Updated the read me with basic usage instructions. Result: New methods that allow for easily creating requests to socket paths, and passing tests to go with them. * Removed some of the new public methods added for creating a socket-path based request Motivation: I previously added too much new public API that will most likely not be necessary, and can be better accessed using a generic execute method. Modifications: Removed the get/post/patch/put/delete methods that were specific to socket paths. Result: Less new public API. * Renamed execute(url:) methods such that the HTTP method is the first argument in the parameter list Motivation: If these are intended to be general methods for building simple requests, then it makes sense to have the method be the first parameter in the list. Modifications: Moved the `method: HTTPMethod` parameter to the front of the list for all `execute([...] url: [...])` methods, and made it default to .GET. I also changed the url parameter to be `urlPath` for the two socketPath based execute methods. Result: A cleaner public interface for users of the API. * Fixed some minor issues introduces with logging Motivation: Some of the convenience request methods weren't properly adapted for logging. Modifications: - Removed a doc comment from patch() that incorrectly referenced a logger. - Fixed an issue where patch() would call into post(). - Added a doc comment to delete() that references the logger. - Tests for the above come in the next commit... Result: Correct documentation and functionality for the patch() and delete() methods. * Updated logging tests to also check the new execute methods Motivation: The logging tests previously didn't check for socket path-based requests. Modifications: Updated the `testAllMethodsLog()` and `testAllMethodsLog()` tests to include checks for each of the new `execute()` methods. Result: Two more tests that pass. --- README.md | 19 ++ Sources/AsyncHTTPClient/HTTPClient.swift | 90 ++++-- Sources/AsyncHTTPClient/HTTPHandler.swift | 34 ++- .../HTTPClientInternalTests+XCTest.swift | 1 + .../HTTPClientInternalTests.swift | 27 ++ .../HTTPClientTestUtils.swift | 5 + .../HTTPClientTests+XCTest.swift | 4 + .../HTTPClientTests.swift | 279 +++++++++++++++++- 8 files changed, 421 insertions(+), 38 deletions(-) diff --git a/README.md b/README.md index a47e2572a..0474f6b17 100644 --- a/README.md +++ b/README.md @@ -157,3 +157,22 @@ httpClient.execute(request: request, delegate: delegate).futureResult.whenSucces print(count) } ``` + +### Unix Domain Socket Paths +Connecting to servers bound to socket paths is easy: +```swift +let httpClient = HTTPClient(eventLoopGroupProvider: .createNew) +httpClient.execute(.GET, socketPath: "/tmp/myServer.socket", urlPath: "/path/to/resource").whenComplete (...) +``` + +Connecting over TLS to a unix domain socket path is possible as well: +```swift +let httpClient = HTTPClient(eventLoopGroupProvider: .createNew) +httpClient.execute(.POST, secureSocketPath: "/tmp/myServer.socket", urlPath: "/path/to/resource", body: .string("hello")).whenComplete (...) +``` + +Direct URLs can easily be contructed to be executed in other scenarios: +```swift +let socketPathBasedURL = URL(httpURLWithSocketPath: "/tmp/myServer.socket", uri: "/path/to/resource") +let secureSocketPathBasedURL = URL(httpsURLWithSocketPath: "/tmp/myServer.socket", uri: "/path/to/resource") +``` diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index faa31d06d..1910157dc 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -237,12 +237,7 @@ public class HTTPClient { /// - deadline: Point in time by which the request must complete. /// - logger: The logger to use for this request. public func get(url: String, deadline: NIODeadline? = nil, logger: Logger) -> EventLoopFuture { - do { - let request = try Request(url: url, method: .GET) - return self.execute(request: request, deadline: deadline, logger: logger) - } catch { - return self.eventLoopGroup.next().makeFailedFuture(error) - } + return self.execute(.GET, url: url, deadline: deadline, logger: logger) } /// Execute `POST` request using specified URL. @@ -263,12 +258,7 @@ public class HTTPClient { /// - deadline: Point in time by which the request must complete. /// - logger: The logger to use for this request. public func post(url: String, body: Body? = nil, deadline: NIODeadline? = nil, logger: Logger) -> EventLoopFuture { - do { - let request = try HTTPClient.Request(url: url, method: .POST, body: body) - return self.execute(request: request, deadline: deadline, logger: logger) - } catch { - return self.eventLoopGroup.next().makeFailedFuture(error) - } + return self.execute(.POST, url: url, body: body, deadline: deadline, logger: logger) } /// Execute `PATCH` request using specified URL. @@ -277,9 +267,8 @@ public class HTTPClient { /// - url: Remote URL. /// - body: Request body. /// - deadline: Point in time by which the request must complete. - /// - logger: The logger to use for this request. public func patch(url: String, body: Body? = nil, deadline: NIODeadline? = nil) -> EventLoopFuture { - return self.post(url: url, body: body, deadline: deadline, logger: HTTPClient.loggingDisabled) + return self.patch(url: url, body: body, deadline: deadline, logger: HTTPClient.loggingDisabled) } /// Execute `PATCH` request using specified URL. @@ -290,12 +279,7 @@ public class HTTPClient { /// - deadline: Point in time by which the request must complete. /// - logger: The logger to use for this request. public func patch(url: String, body: Body? = nil, deadline: NIODeadline? = nil, logger: Logger) -> EventLoopFuture { - do { - let request = try HTTPClient.Request(url: url, method: .PATCH, body: body) - return self.execute(request: request, deadline: deadline, logger: logger) - } catch { - return self.eventLoopGroup.next().makeFailedFuture(error) - } + return self.execute(.PATCH, url: url, body: body, deadline: deadline, logger: logger) } /// Execute `PUT` request using specified URL. @@ -316,12 +300,7 @@ public class HTTPClient { /// - deadline: Point in time by which the request must complete. /// - logger: The logger to use for this request. public func put(url: String, body: Body? = nil, deadline: NIODeadline? = nil, logger: Logger) -> EventLoopFuture { - do { - let request = try HTTPClient.Request(url: url, method: .PUT, body: body) - return self.execute(request: request, deadline: deadline, logger: logger) - } catch { - return self.eventLoopGroup.next().makeFailedFuture(error) - } + return self.execute(.PUT, url: url, body: body, deadline: deadline, logger: logger) } /// Execute `DELETE` request using specified URL. @@ -338,10 +317,65 @@ public class HTTPClient { /// - parameters: /// - url: Remote URL. /// - deadline: The time when the request must have been completed by. + /// - logger: The logger to use for this request. public func delete(url: String, deadline: NIODeadline? = nil, logger: Logger) -> EventLoopFuture { + return self.execute(.DELETE, url: url, deadline: deadline, logger: logger) + } + + /// Execute arbitrary HTTP request using specified URL. + /// + /// - parameters: + /// - method: Request method. + /// - url: Request url. + /// - body: Request body. + /// - deadline: Point in time by which the request must complete. + /// - logger: The logger to use for this request. + public func execute(_ method: HTTPMethod = .GET, url: String, body: Body? = nil, deadline: NIODeadline? = nil, logger: Logger? = nil) -> EventLoopFuture { do { - let request = try Request(url: url, method: .DELETE) - return self.execute(request: request, deadline: deadline, logger: logger) + let request = try Request(url: url, method: method, body: body) + return self.execute(request: request, deadline: deadline, logger: logger ?? HTTPClient.loggingDisabled) + } catch { + return self.eventLoopGroup.next().makeFailedFuture(error) + } + } + + /// Execute arbitrary HTTP+UNIX request to a unix domain socket path, using the specified URL as the request to send to the server. + /// + /// - parameters: + /// - method: Request method. + /// - socketPath: The path to the unix domain socket to connect to. + /// - urlPath: The URL path and query that will be sent to the server. + /// - body: Request body. + /// - deadline: Point in time by which the request must complete. + /// - logger: The logger to use for this request. + public func execute(_ method: HTTPMethod = .GET, socketPath: String, urlPath: String, body: Body? = nil, deadline: NIODeadline? = nil, logger: Logger? = nil) -> EventLoopFuture { + do { + guard let url = URL(httpURLWithSocketPath: socketPath, uri: urlPath) else { + throw HTTPClientError.invalidURL + } + let request = try Request(url: url, method: method, body: body) + return self.execute(request: request, deadline: deadline, logger: logger ?? HTTPClient.loggingDisabled) + } catch { + return self.eventLoopGroup.next().makeFailedFuture(error) + } + } + + /// Execute arbitrary HTTPS+UNIX request to a unix domain socket path over TLS, using the specified URL as the request to send to the server. + /// + /// - parameters: + /// - method: Request method. + /// - secureSocketPath: The path to the unix domain socket to connect to. + /// - urlPath: The URL path and query that will be sent to the server. + /// - body: Request body. + /// - deadline: Point in time by which the request must complete. + /// - logger: The logger to use for this request. + public func execute(_ method: HTTPMethod = .GET, secureSocketPath: String, urlPath: String, body: Body? = nil, deadline: NIODeadline? = nil, logger: Logger? = nil) -> EventLoopFuture { + do { + guard let url = URL(httpsURLWithSocketPath: secureSocketPath, uri: urlPath) else { + throw HTTPClientError.invalidURL + } + let request = try Request(url: url, method: method, body: body) + return self.execute(request: request, deadline: deadline, logger: logger ?? HTTPClient.loggingDisabled) } catch { return self.eventLoopGroup.next().makeFailedFuture(error) } diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index 945f063b3..fb32f482c 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -95,8 +95,8 @@ extension HTTPClient { /// Represent HTTP request. public struct Request { /// Represent kind of Request - enum Kind { - enum UnixScheme { + enum Kind: Equatable { + enum UnixScheme: Equatable { case baseURL case http_unix case https_unix @@ -516,6 +516,36 @@ extension URL { func hasTheSameOrigin(as other: URL) -> Bool { return self.host == other.host && self.scheme == other.scheme && self.port == other.port } + + /// Initializes a newly created HTTP URL connecting to a unix domain socket path. The socket path is encoded as the URL's host, replacing percent encoding invalid path characters, and will use the "http+unix" scheme. + /// - Parameters: + /// - socketPath: The path to the unix domain socket to connect to. + /// - uri: The URI path and query that will be sent to the server. + public init?(httpURLWithSocketPath socketPath: String, uri: String = "/") { + guard let host = socketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed) else { return nil } + var urlString: String + if uri.hasPrefix("/") { + urlString = "http+unix://\(host)\(uri)" + } else { + urlString = "http+unix://\(host)/\(uri)" + } + self.init(string: urlString) + } + + /// Initializes a newly created HTTPS URL connecting to a unix domain socket path over TLS. The socket path is encoded as the URL's host, replacing percent encoding invalid path characters, and will use the "https+unix" scheme. + /// - Parameters: + /// - socketPath: The path to the unix domain socket to connect to. + /// - uri: The URI path and query that will be sent to the server. + public init?(httpsURLWithSocketPath socketPath: String, uri: String = "/") { + guard let host = socketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed) else { return nil } + var urlString: String + if uri.hasPrefix("/") { + urlString = "https+unix://\(host)\(uri)" + } else { + urlString = "https+unix://\(host)/\(uri)" + } + self.init(string: urlString) + } } extension HTTPClient { diff --git a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests+XCTest.swift index 9a3e11b50..8a89eb740 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests+XCTest.swift @@ -43,6 +43,7 @@ extension HTTPClientInternalTests { ("testUploadStreamingIsCalledOnTaskEL", testUploadStreamingIsCalledOnTaskEL), ("testWeCanActuallyExactlySetTheEventLoops", testWeCanActuallyExactlySetTheEventLoops), ("testTaskPromiseBoundToEL", testTaskPromiseBoundToEL), + ("testInternalRequestURI", testInternalRequestURI), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift index e5a6ad411..39d0c6f3a 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift @@ -959,4 +959,31 @@ class HTTPClientInternalTests: XCTestCase { XCTAssertTrue(task.futureResult.eventLoop === el2) XCTAssertNoThrow(try task.wait()) } + + func testInternalRequestURI() throws { + let request1 = try Request(url: "https://someserver.com:8888/some/path?foo=bar") + XCTAssertEqual(request1.kind, .host) + XCTAssertEqual(request1.socketPath, "") + XCTAssertEqual(request1.uri, "/some/path?foo=bar") + + let request2 = try Request(url: "https://someserver.com") + XCTAssertEqual(request2.kind, .host) + XCTAssertEqual(request2.socketPath, "") + XCTAssertEqual(request2.uri, "/") + + let request3 = try Request(url: "unix:///tmp/file") + XCTAssertEqual(request3.kind, .unixSocket(.baseURL)) + XCTAssertEqual(request3.socketPath, "/tmp/file") + XCTAssertEqual(request3.uri, "/") + + let request4 = try Request(url: "http+unix://%2Ftmp%2Ffile/file/path") + XCTAssertEqual(request4.kind, .unixSocket(.http_unix)) + XCTAssertEqual(request4.socketPath, "/tmp/file") + XCTAssertEqual(request4.uri, "/file/path") + + let request5 = try Request(url: "https+unix://%2Ftmp%2Ffile/file/path") + XCTAssertEqual(request5.kind, .unixSocket(.https_unix)) + XCTAssertEqual(request5.socketPath, "/tmp/file") + XCTAssertEqual(request5.uri, "/file/path") + } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index ed65fd7b0..e5df9cfc5 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -439,6 +439,11 @@ internal final class HttpBinHandler: ChannelInboundHandler { headers.add(name: "X-Calling-URI", value: req.uri) self.resps.append(HTTPResponseBuilder(status: .ok, headers: headers)) return + case "/echo-method": + var headers = self.responseHeaders + headers.add(name: "X-Method-Used", value: req.method.rawValue) + self.resps.append(HTTPResponseBuilder(status: .ok, headers: headers)) + return case "/ok": self.resps.append(HTTPResponseBuilder(status: .ok)) return diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 80b0016db..7e64e0a8e 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -28,6 +28,10 @@ extension HTTPClientTests { ("testRequestURI", testRequestURI), ("testBadRequestURI", testBadRequestURI), ("testSchemaCasing", testSchemaCasing), + ("testURLSocketPathInitializers", testURLSocketPathInitializers), + ("testConvenienceExecuteMethods", testConvenienceExecuteMethods), + ("testConvenienceExecuteMethodsOverSocket", testConvenienceExecuteMethodsOverSocket), + ("testConvenienceExecuteMethodsOverSecureSocket", testConvenienceExecuteMethodsOverSecureSocket), ("testGet", testGet), ("testGetWithDifferentEventLoopBackpressure", testGetWithDifferentEventLoopBackpressure), ("testPost", testPost), diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 936c9c286..28c4c2cb3 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -98,6 +98,18 @@ class HTTPClientTests: XCTestCase { XCTAssertEqual(request3.url.path, "/tmp/file") XCTAssertEqual(request3.port, 80) XCTAssertFalse(request3.useTLS) + + let request4 = try Request(url: "http+unix://%2Ftmp%2Ffile/file/path") + XCTAssertEqual(request4.host, "") + XCTAssertEqual(request4.url.host, "/tmp/file") + XCTAssertEqual(request4.url.path, "/file/path") + XCTAssertFalse(request4.useTLS) + + let request5 = try Request(url: "https+unix://%2Ftmp%2Ffile/file/path") + XCTAssertEqual(request5.host, "") + XCTAssertEqual(request5.url.host, "/tmp/file") + XCTAssertEqual(request5.url.path, "/file/path") + XCTAssertTrue(request5.useTLS) } func testBadRequestURI() throws { @@ -110,11 +122,148 @@ class HTTPClientTests: XCTestCase { XCTAssertThrowsError(try Request(url: "https:/foo"), "should throw") { error in XCTAssertEqual(error as! HTTPClientError, HTTPClientError.emptyHost) } + XCTAssertThrowsError(try Request(url: "http+unix:///path"), "should throw") { error in + XCTAssertEqual(error as! HTTPClientError, HTTPClientError.missingSocketPath) + } } func testSchemaCasing() throws { XCTAssertNoThrow(try Request(url: "hTTpS://someserver.com:8888/some/path?foo=bar")) XCTAssertNoThrow(try Request(url: "uNIx:///some/path")) + XCTAssertNoThrow(try Request(url: "hTtP+uNIx://%2Fsome%2Fpath/")) + XCTAssertNoThrow(try Request(url: "hTtPS+uNIx://%2Fsome%2Fpath/")) + } + + func testURLSocketPathInitializers() throws { + let url1 = URL(httpURLWithSocketPath: "/tmp/file") + XCTAssertNotNil(url1) + if let url = url1 { + XCTAssertEqual(url.scheme, "http+unix") + XCTAssertEqual(url.host, "/tmp/file") + XCTAssertEqual(url.path, "/") + XCTAssertEqual(url.absoluteString, "http+unix://%2Ftmp%2Ffile/") + } + + let url2 = URL(httpURLWithSocketPath: "/tmp/file", uri: "/file/path") + XCTAssertNotNil(url2) + if let url = url2 { + XCTAssertEqual(url.scheme, "http+unix") + XCTAssertEqual(url.host, "/tmp/file") + XCTAssertEqual(url.path, "/file/path") + XCTAssertEqual(url.absoluteString, "http+unix://%2Ftmp%2Ffile/file/path") + } + + let url3 = URL(httpURLWithSocketPath: "/tmp/file", uri: "file/path") + XCTAssertNotNil(url3) + if let url = url3 { + XCTAssertEqual(url.scheme, "http+unix") + XCTAssertEqual(url.host, "/tmp/file") + XCTAssertEqual(url.path, "/file/path") + XCTAssertEqual(url.absoluteString, "http+unix://%2Ftmp%2Ffile/file/path") + } + + let url4 = URL(httpURLWithSocketPath: "/tmp/file with spacesと漢字", uri: "file/path") + XCTAssertNotNil(url4) + if let url = url4 { + XCTAssertEqual(url.scheme, "http+unix") + XCTAssertEqual(url.host, "/tmp/file with spacesと漢字") + XCTAssertEqual(url.path, "/file/path") + XCTAssertEqual(url.absoluteString, "http+unix://%2Ftmp%2Ffile%20with%20spaces%E3%81%A8%E6%BC%A2%E5%AD%97/file/path") + } + + let url5 = URL(httpsURLWithSocketPath: "/tmp/file") + XCTAssertNotNil(url5) + if let url = url5 { + XCTAssertEqual(url.scheme, "https+unix") + XCTAssertEqual(url.host, "/tmp/file") + XCTAssertEqual(url.path, "/") + XCTAssertEqual(url.absoluteString, "https+unix://%2Ftmp%2Ffile/") + } + + let url6 = URL(httpsURLWithSocketPath: "/tmp/file", uri: "/file/path") + XCTAssertNotNil(url6) + if let url = url6 { + XCTAssertEqual(url.scheme, "https+unix") + XCTAssertEqual(url.host, "/tmp/file") + XCTAssertEqual(url.path, "/file/path") + XCTAssertEqual(url.absoluteString, "https+unix://%2Ftmp%2Ffile/file/path") + } + + let url7 = URL(httpsURLWithSocketPath: "/tmp/file", uri: "file/path") + XCTAssertNotNil(url7) + if let url = url7 { + XCTAssertEqual(url.scheme, "https+unix") + XCTAssertEqual(url.host, "/tmp/file") + XCTAssertEqual(url.path, "/file/path") + XCTAssertEqual(url.absoluteString, "https+unix://%2Ftmp%2Ffile/file/path") + } + + let url8 = URL(httpsURLWithSocketPath: "/tmp/file with spacesと漢字", uri: "file/path") + XCTAssertNotNil(url8) + if let url = url8 { + XCTAssertEqual(url.scheme, "https+unix") + XCTAssertEqual(url.host, "/tmp/file with spacesと漢字") + XCTAssertEqual(url.path, "/file/path") + XCTAssertEqual(url.absoluteString, "https+unix://%2Ftmp%2Ffile%20with%20spaces%E3%81%A8%E6%BC%A2%E5%AD%97/file/path") + } + + let url9 = URL(httpURLWithSocketPath: "/tmp/file", uri: " ") + XCTAssertNil(url9) + + let url10 = URL(httpsURLWithSocketPath: "/tmp/file", uri: " ") + XCTAssertNil(url10) + } + + func testConvenienceExecuteMethods() throws { + XCTAssertNoThrow(XCTAssertEqual(["GET"[...]], + try self.defaultClient.get(url: self.defaultHTTPBinURLPrefix + "echo-method").wait().headers[canonicalForm: "X-Method-Used"])) + XCTAssertNoThrow(XCTAssertEqual(["POST"[...]], + try self.defaultClient.post(url: self.defaultHTTPBinURLPrefix + "echo-method").wait().headers[canonicalForm: "X-Method-Used"])) + XCTAssertNoThrow(XCTAssertEqual(["PATCH"[...]], + try self.defaultClient.patch(url: self.defaultHTTPBinURLPrefix + "echo-method").wait().headers[canonicalForm: "X-Method-Used"])) + XCTAssertNoThrow(XCTAssertEqual(["PUT"[...]], + try self.defaultClient.put(url: self.defaultHTTPBinURLPrefix + "echo-method").wait().headers[canonicalForm: "X-Method-Used"])) + XCTAssertNoThrow(XCTAssertEqual(["DELETE"[...]], + try self.defaultClient.delete(url: self.defaultHTTPBinURLPrefix + "echo-method").wait().headers[canonicalForm: "X-Method-Used"])) + XCTAssertNoThrow(XCTAssertEqual(["GET"[...]], + try self.defaultClient.execute(url: self.defaultHTTPBinURLPrefix + "echo-method").wait().headers[canonicalForm: "X-Method-Used"])) + XCTAssertNoThrow(XCTAssertEqual(["CHECKOUT"[...]], + try self.defaultClient.execute(.CHECKOUT, url: self.defaultHTTPBinURLPrefix + "echo-method").wait().headers[canonicalForm: "X-Method-Used"])) + } + + func testConvenienceExecuteMethodsOverSocket() throws { + XCTAssertNoThrow(try TemporaryFileHelpers.withTemporaryUnixDomainSocketPathName { path in + let localSocketPathHTTPBin = HTTPBin(bindTarget: .unixDomainSocket(path)) + defer { + XCTAssertNoThrow(try localSocketPathHTTPBin.shutdown()) + } + + XCTAssertNoThrow(XCTAssertEqual(["GET"[...]], + try self.defaultClient.execute(socketPath: path, urlPath: "echo-method").wait().headers[canonicalForm: "X-Method-Used"])) + XCTAssertNoThrow(XCTAssertEqual(["GET"[...]], + try self.defaultClient.execute(.GET, socketPath: path, urlPath: "echo-method").wait().headers[canonicalForm: "X-Method-Used"])) + XCTAssertNoThrow(XCTAssertEqual(["POST"[...]], + try self.defaultClient.execute(.POST, socketPath: path, urlPath: "echo-method").wait().headers[canonicalForm: "X-Method-Used"])) + }) + } + + func testConvenienceExecuteMethodsOverSecureSocket() throws { + XCTAssertNoThrow(try TemporaryFileHelpers.withTemporaryUnixDomainSocketPathName { path in + let localSocketPathHTTPBin = HTTPBin(ssl: true, bindTarget: .unixDomainSocket(path)) + let localClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), + configuration: HTTPClient.Configuration(certificateVerification: .none)) + defer { + XCTAssertNoThrow(try localClient.syncShutdown()) + XCTAssertNoThrow(try localSocketPathHTTPBin.shutdown()) + } + + XCTAssertNoThrow(XCTAssertEqual(["GET"[...]], + try localClient.execute(secureSocketPath: path, urlPath: "echo-method").wait().headers[canonicalForm: "X-Method-Used"])) + XCTAssertNoThrow(XCTAssertEqual(["GET"[...]], + try localClient.execute(.GET, secureSocketPath: path, urlPath: "echo-method").wait().headers[canonicalForm: "X-Method-Used"])) + XCTAssertNoThrow(XCTAssertEqual(["POST"[...]], + try localClient.execute(.POST, secureSocketPath: path, urlPath: "echo-method").wait().headers[canonicalForm: "X-Method-Used"])) + }) } func testGet() throws { @@ -1351,7 +1500,7 @@ class HTTPClientTests: XCTestCase { defer { XCTAssertNoThrow(try localHTTPBin.shutdown()) } - guard let target = URL(string: "http+unix://\(path.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/echo-uri"), + guard let target = URL(httpURLWithSocketPath: path, uri: "/echo-uri"), let request = try? Request(url: target) else { XCTFail("couldn't build URL for request") return @@ -1371,7 +1520,7 @@ class HTTPClientTests: XCTestCase { XCTAssertNoThrow(try localClient.syncShutdown()) XCTAssertNoThrow(try localHTTPBin.shutdown()) } - guard let target = URL(string: "https+unix://\(path.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/echo-uri"), + guard let target = URL(httpsURLWithSocketPath: path, uri: "/echo-uri"), let request = try? Request(url: target) else { XCTFail("couldn't build URL for request") return @@ -1942,7 +2091,70 @@ class HTTPClientTests: XCTestCase { logger: logger).wait()) XCTAssertEqual(0, logStore.allEntries.count) + // === Synthesized Request + XCTAssertNoThrow(try self.defaultClient.execute(.GET, + url: self.defaultHTTPBinURLPrefix + "get", + body: nil, + deadline: nil, + logger: logger).wait()) + XCTAssertEqual(0, logStore.allEntries.count) + XCTAssertEqual(0, self.backgroundLogStore.allEntries.count) + + // === Synthesized Socket Path Request + XCTAssertNoThrow(try TemporaryFileHelpers.withTemporaryUnixDomainSocketPathName { path in + let backgroundLogStore = CollectEverythingLogHandler.LogStore() + var backgroundLogger = Logger(label: "\(#function)", factory: { _ in + CollectEverythingLogHandler(logStore: backgroundLogStore) + }) + backgroundLogger.logLevel = .trace + + let localSocketPathHTTPBin = HTTPBin(bindTarget: .unixDomainSocket(path)) + let localClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), + backgroundActivityLogger: backgroundLogger) + defer { + XCTAssertNoThrow(try localClient.syncShutdown()) + XCTAssertNoThrow(try localSocketPathHTTPBin.shutdown()) + } + + XCTAssertNoThrow(try localClient.execute(.GET, + socketPath: path, + urlPath: "get", + body: nil, + deadline: nil, + logger: logger).wait()) + XCTAssertEqual(0, logStore.allEntries.count) + + XCTAssertEqual(0, backgroundLogStore.allEntries.count) + }) + + // === Synthesized Secure Socket Path Request + XCTAssertNoThrow(try TemporaryFileHelpers.withTemporaryUnixDomainSocketPathName { path in + let backgroundLogStore = CollectEverythingLogHandler.LogStore() + var backgroundLogger = Logger(label: "\(#function)", factory: { _ in + CollectEverythingLogHandler(logStore: backgroundLogStore) + }) + backgroundLogger.logLevel = .trace + + let localSocketPathHTTPBin = HTTPBin(ssl: true, bindTarget: .unixDomainSocket(path)) + let localClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), + configuration: HTTPClient.Configuration(certificateVerification: .none), + backgroundActivityLogger: backgroundLogger) + defer { + XCTAssertNoThrow(try localClient.syncShutdown()) + XCTAssertNoThrow(try localSocketPathHTTPBin.shutdown()) + } + + XCTAssertNoThrow(try localClient.execute(.GET, + secureSocketPath: path, + urlPath: "get", + body: nil, + deadline: nil, + logger: logger).wait()) + XCTAssertEqual(0, logStore.allEntries.count) + + XCTAssertEqual(0, backgroundLogStore.allEntries.count) + }) } func testAllMethodsLog() { @@ -1955,7 +2167,7 @@ class HTTPClientTests: XCTestCase { logger.logLevel = .trace logger[metadataKey: "req"] = "yo-\(type)" - let url = self.defaultHTTPBinURLPrefix + "not-found/request/\(type))" + let url = "not-found/request/\(type))" let result = try body(logger, url) XCTAssertGreaterThan(logStore.allEntries.count, 0) @@ -1967,27 +2179,78 @@ class HTTPClientTests: XCTestCase { } XCTAssertNoThrow(XCTAssertEqual(.notFound, try checkExpectationsWithLogger(type: "GET") { logger, url in - try self.defaultClient.get(url: url, logger: logger).wait() + try self.defaultClient.get(url: self.defaultHTTPBinURLPrefix + url, logger: logger).wait() }.status)) XCTAssertNoThrow(XCTAssertEqual(.notFound, try checkExpectationsWithLogger(type: "PUT") { logger, url in - try self.defaultClient.put(url: url, logger: logger).wait() + try self.defaultClient.put(url: self.defaultHTTPBinURLPrefix + url, logger: logger).wait() }.status)) XCTAssertNoThrow(XCTAssertEqual(.notFound, try checkExpectationsWithLogger(type: "POST") { logger, url in - try self.defaultClient.post(url: url, logger: logger).wait() + try self.defaultClient.post(url: self.defaultHTTPBinURLPrefix + url, logger: logger).wait() }.status)) XCTAssertNoThrow(XCTAssertEqual(.notFound, try checkExpectationsWithLogger(type: "DELETE") { logger, url in - try self.defaultClient.delete(url: url, logger: logger).wait() + try self.defaultClient.delete(url: self.defaultHTTPBinURLPrefix + url, logger: logger).wait() }.status)) XCTAssertNoThrow(XCTAssertEqual(.notFound, try checkExpectationsWithLogger(type: "PATCH") { logger, url in - try self.defaultClient.patch(url: url, logger: logger).wait() + try self.defaultClient.patch(url: self.defaultHTTPBinURLPrefix + url, logger: logger).wait() + }.status)) + + XCTAssertNoThrow(XCTAssertEqual(.notFound, try checkExpectationsWithLogger(type: "CHECKOUT") { logger, url in + try self.defaultClient.execute(.CHECKOUT, url: self.defaultHTTPBinURLPrefix + url, logger: logger).wait() }.status)) // No background activity expected here. XCTAssertEqual(0, self.backgroundLogStore.allEntries.count) + + XCTAssertNoThrow(try TemporaryFileHelpers.withTemporaryUnixDomainSocketPathName { path in + let backgroundLogStore = CollectEverythingLogHandler.LogStore() + var backgroundLogger = Logger(label: "\(#function)", factory: { _ in + CollectEverythingLogHandler(logStore: backgroundLogStore) + }) + backgroundLogger.logLevel = .trace + + let localSocketPathHTTPBin = HTTPBin(bindTarget: .unixDomainSocket(path)) + let localClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), + backgroundActivityLogger: backgroundLogger) + defer { + XCTAssertNoThrow(try localClient.syncShutdown()) + XCTAssertNoThrow(try localSocketPathHTTPBin.shutdown()) + } + + XCTAssertNoThrow(XCTAssertEqual(.notFound, try checkExpectationsWithLogger(type: "GET") { logger, url in + try localClient.execute(socketPath: path, urlPath: url, logger: logger).wait() + }.status)) + + // No background activity expected here. + XCTAssertEqual(0, backgroundLogStore.allEntries.count) + }) + + XCTAssertNoThrow(try TemporaryFileHelpers.withTemporaryUnixDomainSocketPathName { path in + let backgroundLogStore = CollectEverythingLogHandler.LogStore() + var backgroundLogger = Logger(label: "\(#function)", factory: { _ in + CollectEverythingLogHandler(logStore: backgroundLogStore) + }) + backgroundLogger.logLevel = .trace + + let localSocketPathHTTPBin = HTTPBin(ssl: true, bindTarget: .unixDomainSocket(path)) + let localClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), + configuration: HTTPClient.Configuration(certificateVerification: .none), + backgroundActivityLogger: backgroundLogger) + defer { + XCTAssertNoThrow(try localClient.syncShutdown()) + XCTAssertNoThrow(try localSocketPathHTTPBin.shutdown()) + } + + XCTAssertNoThrow(XCTAssertEqual(.notFound, try checkExpectationsWithLogger(type: "GET") { logger, url in + try localClient.execute(secureSocketPath: path, urlPath: url, logger: logger).wait() + }.status)) + + // No background activity expected here. + XCTAssertEqual(0, backgroundLogStore.allEntries.count) + }) } func testClosingIdleConnectionsInPoolLogsInTheBackground() { From 9a431d17b2857cc89ff6b1efcb88586977d3c79e Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Fri, 19 Jun 2020 16:34:11 +0100 Subject: [PATCH 04/12] wip --- Sources/AsyncHTTPClient/HTTPClient.swift | 3 + .../AsyncHTTPClient/RequestValidation.swift | 4 + .../HTTPClientTests.swift | 2 +- .../RequestValidationTests+XCTest.swift | 2 - .../RequestValidationTests.swift | 211 +++++++++++++++--- 5 files changed, 187 insertions(+), 35 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 1910157dc..26f5321b8 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -960,6 +960,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible { case traceRequestWithBody case invalidHeaderFieldNames([String]) case bodyLengthMismatch + case incompatibleHeaders } private var code: Code @@ -1012,4 +1013,6 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible { public static func invalidHeaderFieldNames(_ names: [String]) -> HTTPClientError { return HTTPClientError(code: .invalidHeaderFieldNames(names)) } /// Body length is not equal to `Content-Length`. public static let bodyLengthMismatch = HTTPClientError(code: .bodyLengthMismatch) + /// Incompatible headers specified, for example `Transfer-Encoding` and `Content-Length`. + public static let incompatibleHeaders = HTTPClientError(code: .incompatibleHeaders) } diff --git a/Sources/AsyncHTTPClient/RequestValidation.swift b/Sources/AsyncHTTPClient/RequestValidation.swift index c01229015..6e7447cd9 100644 --- a/Sources/AsyncHTTPClient/RequestValidation.swift +++ b/Sources/AsyncHTTPClient/RequestValidation.swift @@ -18,6 +18,10 @@ import NIOHTTP1 extension HTTPHeaders { mutating func validate(method: HTTPMethod, body: HTTPClient.Body?) throws { // validate transfer encoding and content length (https://tools.ietf.org/html/rfc7230#section-3.3.1) + if self.contains(name: "Transfer-Encoding") && self.contains(name: "Content-Length") { + throw HTTPClientError.incompatibleHeaders + } + var transferEncoding: String? var contentLength: Int? let encodings = self[canonicalForm: "Transfer-Encoding"].map { $0.lowercased() } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 28c4c2cb3..01a1648fb 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2281,7 +2281,7 @@ class HTTPClientTests: XCTestCase { var request = try HTTPClient.Request(url: "http://localhost:\(server.serverPort)/") request.body = .stream { writer in - writer.write(.byteBuffer(ByteBuffer.of(string: "1234"))) + writer.write(.byteBuffer(ByteBuffer(string: "1234"))) } let future = client.execute(request: request) diff --git a/Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift b/Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift index 51e6ff016..1b0fa7814 100644 --- a/Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift @@ -28,12 +28,10 @@ extension RequestValidationTests { ("testContentLengthHeaderIsRemovedFromGETIfNoBody", testContentLengthHeaderIsRemovedFromGETIfNoBody), ("testContentLengthHeaderIsAddedToPOSTAndPUTWithNoBody", testContentLengthHeaderIsAddedToPOSTAndPUTWithNoBody), ("testContentLengthHeaderIsChangedIfBodyHasDifferentLength", testContentLengthHeaderIsChangedIfBodyHasDifferentLength), - ("testChunkedEncodingDoesNotHaveContentLengthHeader", testChunkedEncodingDoesNotHaveContentLengthHeader), ("testTRACERequestMustNotHaveBody", testTRACERequestMustNotHaveBody), ("testGET_HEAD_DELETE_CONNECTRequestCanHaveBody", testGET_HEAD_DELETE_CONNECTRequestCanHaveBody), ("testInvalidHeaderFieldNames", testInvalidHeaderFieldNames), ("testValidHeaderFieldNames", testValidHeaderFieldNames), - ("testMultipleContentLengthOnNilStreamLength", testMultipleContentLengthOnNilStreamLength), ] } } diff --git a/Tests/AsyncHTTPClientTests/RequestValidationTests.swift b/Tests/AsyncHTTPClientTests/RequestValidationTests.swift index fe3cb9330..edf0bb736 100644 --- a/Tests/AsyncHTTPClientTests/RequestValidationTests.swift +++ b/Tests/AsyncHTTPClientTests/RequestValidationTests.swift @@ -42,32 +42,14 @@ class RequestValidationTests: XCTestCase { XCTAssertEqual(headers.first(name: "Content-Length"), "200") } - func testChunkedEncodingDoesNotHaveContentLengthHeader() { - var headers = HTTPHeaders([ - ("Content-Length", "200"), - ("Transfer-Encoding", "chunked"), - ]) - var buffer = ByteBufferAllocator().buffer(capacity: 200) - buffer.writeBytes([UInt8](repeating: 12, count: 200)) - XCTAssertNoThrow(try headers.validate(method: .PUT, body: .byteBuffer(buffer))) - - // https://tools.ietf.org/html/rfc7230#section-3.3.2 - // A sender MUST NOT send a Content-Length header field in any message - // that contains a Transfer-Encoding header field. - - XCTAssertNil(headers.first(name: "Content-Length")) - XCTAssertEqual(headers.first(name: "Transfer-Encoding"), "chunked") - } - func testTRACERequestMustNotHaveBody() { - var headers = HTTPHeaders([ - ("Content-Length", "200"), - ("Transfer-Encoding", "chunked"), - ]) - var buffer = ByteBufferAllocator().buffer(capacity: 200) - buffer.writeBytes([UInt8](repeating: 12, count: 200)) - XCTAssertThrowsError(try headers.validate(method: .TRACE, body: .byteBuffer(buffer))) { - XCTAssertEqual($0 as? HTTPClientError, .traceRequestWithBody) + for header in [("Content-Length", "200"), ("Transfer-Encoding", "chunked")] { + var headers = HTTPHeaders([header]) + var buffer = ByteBufferAllocator().buffer(capacity: 200) + buffer.writeBytes([UInt8](repeating: 12, count: 200)) + XCTAssertThrowsError(try headers.validate(method: .TRACE, body: .byteBuffer(buffer))) { + XCTAssertEqual($0 as? HTTPClientError, .traceRequestWithBody) + } } } @@ -105,13 +87,178 @@ class RequestValidationTests: XCTestCase { XCTAssertNoThrow(try headers.validate(method: .GET, body: nil)) } - func testMultipleContentLengthOnNilStreamLength() { - var headers = HTTPHeaders([("Content-Length", "1"), ("Content-Length", "2")]) - var buffer = ByteBufferAllocator().buffer(capacity: 10) - buffer.writeBytes([UInt8](repeating: 12, count: 10)) - let body: HTTPClient.Body = .stream { writer in - writer.write(.byteBuffer(buffer)) + // MARK: - Content-Length/Transfer-Encoding Matrix + + // Method kind User sets Body Expectation + // ---------------------------------------------------------------------------------- + // .GET, .HEAD, .DELETE, .CONNECT, .TRACE nothing nil Neither CL nor chunked + // other nothing nil chunked + func testNoHeadersNoBody() throws { + for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT, .TRACE] { + var headers: HTTPHeaders = .init() + XCTAssertNoThrow(try headers.validate(method: method, body: nil)) + XCTAssertTrue(headers["content-length"].isEmpty) + XCTAssertTrue(headers["transfer-encoding"].isEmpty) + } + + for method: HTTPMethod in [.POST, .PUT] { + var headers: HTTPHeaders = .init() + XCTAssertNoThrow(try headers.validate(method: method, body: nil)) + // TODO: This should be CL=0 ??? https://tools.ietf.org/html/rfc7230#section-3.3.2 + XCTAssertTrue(headers["content-length"].isEmpty) + XCTAssertTrue(headers["transfer-encoding"].contains("chunked")) + } + } + + // Method kind User sets Body Expectation + // -------------------------------------------------------------------------------------- + // .GET, .HEAD, .DELETE, .CONNECT, .TRACE nothing not nil Neither CL nor chunked + // other nothing not nil chunked + func testNoHeadersHasBody() throws { + for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT] { + var headers: HTTPHeaders = .init() + XCTAssertNoThrow(try headers.validate(method: method, body: .byteBuffer(ByteBuffer(bytes: [0])))) + + // TODO: Logically, this should be a Content-Length: 1 + + XCTAssertTrue(headers["content-length"].isEmpty) + XCTAssertTrue(headers["transfer-encoding"].isEmpty) + } + + for method: HTTPMethod in [.POST, .PUT] { + var headers: HTTPHeaders = .init() + XCTAssertNoThrow(try headers.validate(method: method, body: .byteBuffer(ByteBuffer(bytes: [0])))) + + // TODO: Logically, this should be a Content-Length: 1 since we know size, or chunked if we don't + + XCTAssertTrue(headers["content-length"].isEmpty) + XCTAssertTrue(headers["transfer-encoding"].contains("chunked")) + } + + for method: HTTPMethod in [.POST, .PUT] { + var headers: HTTPHeaders = .init() + let body: HTTPClient.Body = .stream { writer in + writer.write(.byteBuffer(ByteBuffer(bytes: [0]))) + } + XCTAssertNoThrow(try headers.validate(method: method, body: body)) + + // TODO: Logically, this should be a Content-Length: 1 since we know size, or chunked if we don't + + XCTAssertTrue(headers["content-length"].isEmpty) + XCTAssertTrue(headers["transfer-encoding"].contains("chunked")) + } + } + + // Method kind User sets Body Expectation + // ------------------------------------------------------------------------------ + // .GET, .HEAD, .DELETE, .CONNECT, .TRACE content-length nil CL=0 + // other content-length nil CL=0 + func testContentLengthHeaderNoBody() throws { + for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT, .TRACE] { + var headers: HTTPHeaders = .init([("Content-Length", "1")]) + XCTAssertNoThrow(try headers.validate(method: method, body: nil)) + // TODO: this should be Neither CL nor chunked https://tools.ietf.org/html/rfc7230#section-3.3.2 + XCTAssertEqual(headers["content-length"].first, "0") + XCTAssertTrue(headers["transfer-encoding"].isEmpty) + } + + for method: HTTPMethod in [.POST, .PUT] { + var headers: HTTPHeaders = .init([("Content-Length", "1")]) + XCTAssertNoThrow(try headers.validate(method: method, body: nil)) + XCTAssertEqual(headers["content-length"].first, "0") + XCTAssertTrue(headers["transfer-encoding"].isEmpty) + } + } + + // Method kind User sets Body Expectation + // ---------------------------------------------------------------------------------- + // .GET, .HEAD, .DELETE, .CONNECT, .TRACE content-length not nil CL=1 + // other content-length nit nil CL=1 + func testContentLengthHeaderHasBody() throws { + for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT] { + var headers: HTTPHeaders = .init([("Content-Length", "1")]) + XCTAssertNoThrow(try headers.validate(method: method, body: .byteBuffer(ByteBuffer(bytes: [0])))) + XCTAssertEqual(headers["content-length"].first, "1") + XCTAssertTrue(headers["transfer-encoding"].isEmpty) + } + + for method: HTTPMethod in [.POST, .PUT] { + var headers: HTTPHeaders = .init([("Content-Length", "1")]) + XCTAssertNoThrow(try headers.validate(method: method, body: .byteBuffer(ByteBuffer(bytes: [0])))) + XCTAssertEqual(headers["content-length"].first, "1") + XCTAssertTrue(headers["transfer-encoding"].isEmpty) + } + } + + // Method kind User sets Body Expectation + // ------------------------------------------------------------------------------------------ + // .GET, .HEAD, .DELETE, .CONNECT, .TRACE transfer-encoding: chunked nil chunked + // other transfer-encoding: chunked nil chunked + func testTransferEncodingHeaderNoBody() throws { + for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT, .TRACE] { + var headers: HTTPHeaders = .init([("Transfer-Encoding", "chunked")]) + XCTAssertNoThrow(try headers.validate(method: method, body: nil)) + XCTAssertTrue(headers["content-length"].isEmpty) + XCTAssertTrue(headers["transfer-encoding"].contains("chunked")) + } + + for method: HTTPMethod in [.POST, .PUT] { + var headers: HTTPHeaders = .init([("Transfer-Encoding", "chunked")]) + XCTAssertNoThrow(try headers.validate(method: method, body: nil)) + XCTAssertTrue(headers["content-length"].isEmpty) + XCTAssertTrue(headers["transfer-encoding"].contains("chunked")) + } + } + + // Method kind User sets Body Expectation + // -------------------------------------------------------------------------------------- + // .GET, .HEAD, .DELETE, .CONNECT transfer-encoding: chunked not nil chunked + // other transfer-encoding: chunked not nil chunked + func testTransferEncodingHeaderHasBody() throws { + for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT] { + var headers: HTTPHeaders = .init([("Transfer-Encoding", "chunked")]) + XCTAssertNoThrow(try headers.validate(method: method, body: .byteBuffer(ByteBuffer(bytes: [0])))) + XCTAssertTrue(headers["content-length"].isEmpty) + XCTAssertTrue(headers["transfer-encoding"].contains("chunked")) + } + + for method: HTTPMethod in [.POST, .PUT] { + var headers: HTTPHeaders = .init([("Transfer-Encoding", "chunked")]) + XCTAssertNoThrow(try headers.validate(method: method, body: .byteBuffer(ByteBuffer(bytes: [0])))) + XCTAssertTrue(headers["content-length"].isEmpty) + XCTAssertTrue(headers["transfer-encoding"].contains("chunked")) + } + } + + // Method kind User sets Body Expectation + // --------------------------------------------------------------------------------------- + // .GET, .HEAD, .DELETE, .CONNECT, .TRACE CL & chunked (illegal) nil throws error + // other CL & chunked (illegal) nil throws error + func testBothHeadersNoBody() throws { + for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT, .TRACE] { + var headers: HTTPHeaders = .init([("Content-Length", "1"), ("Transfer-Encoding", "chunked")]) + XCTAssertThrowsError(try headers.validate(method: method, body: nil)) + } + + for method: HTTPMethod in [.POST, .PUT] { + var headers: HTTPHeaders = .init([("Content-Length", "1"), ("Transfer-Encoding", "chunked")]) + XCTAssertThrowsError(try headers.validate(method: method, body: nil)) + } + } + + // Method kind User sets Body Expectation + // ------------------------------------------------------------------------------------------- + // .GET, .HEAD, .DELETE, .CONNECT, .TRACE CL & chunked (illegal) not nil throws error + // other CL & chunked (illegal) not nil throws error + func testBothHeadersHasBody() throws { + for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT, .TRACE] { + var headers: HTTPHeaders = .init([("Content-Length", "1"), ("Transfer-Encoding", "chunked")]) + XCTAssertThrowsError(try headers.validate(method: method, body: .byteBuffer(ByteBuffer(bytes: [0])))) + } + + for method: HTTPMethod in [.POST, .PUT] { + var headers: HTTPHeaders = .init([("Content-Length", "1"), ("Transfer-Encoding", "chunked")]) + XCTAssertThrowsError(try headers.validate(method: method, body: .byteBuffer(ByteBuffer(bytes: [0])))) } - XCTAssertThrowsError(try headers.validate(method: .PUT, body: body)) } } From eaf203ef1a4ecd9ea390008ccb5d9a9bec6ec6eb Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Mon, 22 Jun 2020 15:42:37 +0100 Subject: [PATCH 05/12] fix flaky testContentLengthTooLongFails test (#269) --- Sources/AsyncHTTPClient/HTTPHandler.swift | 6 ++++-- Tests/AsyncHTTPClientTests/HTTPClientTests.swift | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index fb32f482c..d152e6660 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -867,15 +867,17 @@ extension TaskHandler: ChannelDuplexHandler { return context.eventLoop.makeSucceededFuture(()) } + let channel = context.channel + func doIt() -> EventLoopFuture { return body.stream(HTTPClient.Body.StreamWriter { part in let promise = self.task.eventLoop.makePromise(of: Void.self) // All writes have to be switched to the channel EL if channel and task ELs differ - if context.eventLoop.inEventLoop { + if channel.eventLoop.inEventLoop { self.actualBodyLength += part.readableBytes context.writeAndFlush(self.wrapOutboundOut(.body(part)), promise: promise) } else { - context.eventLoop.execute { + channel.eventLoop.execute { self.actualBodyLength += part.readableBytes context.writeAndFlush(self.wrapOutboundOut(.body(part)), promise: promise) } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 01a1648fb..4263863aa 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2350,7 +2350,7 @@ class HTTPClientTests: XCTestCase { } func testContentLengthTooLongFails() throws { - let url = self.defaultHTTPBinURLPrefix + "/post" + let url = self.defaultHTTPBinURLPrefix + "post" XCTAssertThrowsError( try self.defaultClient.execute(request: Request(url: url, @@ -2379,7 +2379,7 @@ class HTTPClientTests: XCTestCase { // currently gets stuck because of #250 the server just never replies func testContentLengthTooShortFails() throws { - let url = self.defaultHTTPBinURLPrefix + "/post" + let url = self.defaultHTTPBinURLPrefix + "post" let tooLong = "XBAD BAD BAD NOT HTTP/1.1\r\n\r\n" XCTAssertThrowsError( try self.defaultClient.execute(request: From 02b3ed92d554ad2ac96106bf06f8f31917171ddb Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Mon, 22 Jun 2020 16:11:28 +0100 Subject: [PATCH 06/12] All internal connection flow should be executed when shutting down (#268) --- Sources/AsyncHTTPClient/ConnectionPool.swift | 26 ++-- .../AsyncHTTPClient/ConnectionsState.swift | 18 ++- .../ConnectionPoolTests+XCTest.swift | 4 + .../ConnectionPoolTests.swift | 145 ++++++++++++++++++ 4 files changed, 176 insertions(+), 17 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool.swift b/Sources/AsyncHTTPClient/ConnectionPool.swift index f18a30a98..ce648469c 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool.swift @@ -374,15 +374,6 @@ class HTTP1ConnectionProvider { "ahc-action": "\(action)"]) connection.channel.close(promise: nil) self.execute(action, logger: logger) - case .cancel(let connection, let close): - logger.trace("cancelling connection", - metadata: ["ahc-connection": "\(connection)", - "ahc-close": "\(close)"]) - connection.cancel().whenComplete { _ in - if close { - self.closeAndDelete() - } - } case .fail(let waiter, let error): logger.debug("failing connection for waiter", metadata: ["ahc-waiter": "\(waiter)", @@ -428,7 +419,17 @@ class HTTP1ConnectionProvider { } return self.state.offer(connection: connection) } - waiter.promise.succeed(connection) + + switch action { + case .closeAnd: + // This happens when client was shut down during connect + logger.trace("connection cancelled due to client shutdown", + metadata: ["ahc-connection": "\(channel)"]) + connection.channel.close(promise: nil) + waiter.promise.fail(HTTPClientError.cancelled) + default: + waiter.promise.succeed(connection) + } case .failure(let error): logger.debug("connection attempt failed", metadata: ["ahc-error": "\(error)"]) @@ -437,6 +438,7 @@ class HTTP1ConnectionProvider { } waiter.promise.fail(error) } + waiter.setupComplete.whenComplete { _ in self.execute(action, logger: logger) } @@ -456,7 +458,7 @@ class HTTP1ConnectionProvider { // Since both `.park` and `.deleteProvider` are terminal in terms of execution, // we can execute them immediately self.execute(action, logger: logger) - case .cancel, .closeAnd, .create, .fail, .lease, .parkAnd, .replace: + case .closeAnd, .create, .fail, .lease, .parkAnd, .replace: // This is needed to start a new stack, otherwise, since this is called on a previous // future completion handler chain, it will be growing indefinitely until the connection is closed. // We might revisit this when https://github.com/apple/swift-nio/issues/970 is resolved. @@ -493,7 +495,7 @@ class HTTP1ConnectionProvider { $0.promise.fail(HTTPClientError.cancelled) } - if available.isEmpty, leased.isEmpty { + if available.isEmpty, leased.isEmpty, clean { self.closePromise.succeed(()) return self.closePromise.futureResult.map { clean } } diff --git a/Sources/AsyncHTTPClient/ConnectionsState.swift b/Sources/AsyncHTTPClient/ConnectionsState.swift index 0c5f908e2..d99416a49 100644 --- a/Sources/AsyncHTTPClient/ConnectionsState.swift +++ b/Sources/AsyncHTTPClient/ConnectionsState.swift @@ -23,7 +23,6 @@ extension HTTP1ConnectionProvider { case park(Connection) case none case fail(Waiter, Error) - case cancel(Connection, Bool) indirect case closeAnd(Connection, Action) indirect case parkAnd(Connection, Action) } @@ -209,8 +208,9 @@ extension HTTP1ConnectionProvider { case .active: self.leasedConnections.insert(ConnectionKey(connection)) return .none - case .closed: // This can happen when we close the client while connections was being estableshed - return .cancel(connection, self.isEmpty) + case .closed: // This can happen when we close the client while connections was being established + self.openedConnectionsCount -= 1 + return .closeAnd(connection, self.processNextWaiter()) } } @@ -233,7 +233,9 @@ extension HTTP1ConnectionProvider { // user calls `syncShutdown` before we received an error from the bootstrap. In this scenario, // pool will be `.closed` but connection will be still in the process of being established/failed, // so then this process finishes, it will get to this point. - return .none + // We need to call `processNextWaiter` to finish deleting provider from the pool. + self.openedConnectionsCount -= 1 + return self.processNextWaiter() } } @@ -273,7 +275,13 @@ extension HTTP1ConnectionProvider { return .closeAnd(connection, self.processNextWaiter()) case .closed: - return .none + // This situation can happen when we call close, state changes, but before we call `close` on all + // available connections, in this case we should close this connection and, potentially, + // delete the provider + self.openedConnectionsCount -= 1 + self.availableConnections.removeAll { $0 === connection } + + return .closeAnd(connection, self.processNextWaiter()) } } diff --git a/Tests/AsyncHTTPClientTests/ConnectionPoolTests+XCTest.swift b/Tests/AsyncHTTPClientTests/ConnectionPoolTests+XCTest.swift index b751ec1ce..04cfa6421 100644 --- a/Tests/AsyncHTTPClientTests/ConnectionPoolTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/ConnectionPoolTests+XCTest.swift @@ -62,6 +62,10 @@ extension ConnectionPoolTests { ("testConnectionRemoteCloseRelease", testConnectionRemoteCloseRelease), ("testConnectionTimeoutRelease", testConnectionTimeoutRelease), ("testAcquireAvailableBecomesUnavailable", testAcquireAvailableBecomesUnavailable), + ("testShutdownOnPendingAndSuccess", testShutdownOnPendingAndSuccess), + ("testShutdownOnPendingAndError", testShutdownOnPendingAndError), + ("testShutdownTimeout", testShutdownTimeout), + ("testShutdownRemoteClosed", testShutdownRemoteClosed), ] } } diff --git a/Tests/AsyncHTTPClientTests/ConnectionPoolTests.swift b/Tests/AsyncHTTPClientTests/ConnectionPoolTests.swift index bacc04dd8..aee407b28 100644 --- a/Tests/AsyncHTTPClientTests/ConnectionPoolTests.swift +++ b/Tests/AsyncHTTPClientTests/ConnectionPoolTests.swift @@ -1391,6 +1391,151 @@ class ConnectionPoolTests: XCTestCase { } } + // MARK: - Shutdown tests + + func testShutdownOnPendingAndSuccess() { + var state = HTTP1ConnectionProvider.ConnectionsState(eventLoop: self.eventLoop) + + XCTAssertTrue(state.enqueue()) + + let connectionPromise = self.eventLoop.makePromise(of: Connection.self) + let setupPromise = self.eventLoop.makePromise(of: Void.self) + let waiter = HTTP1ConnectionProvider.Waiter(promise: connectionPromise, setupComplete: setupPromise.futureResult, preference: .indifferent) + var action = state.acquire(waiter: waiter) + + guard case .create = action else { + XCTFail("unexpected action \(action)") + return + } + + let snapshot = state.testsOnly_getInternalState() + XCTAssertEqual(snapshot.openedConnectionsCount, 1) + + if let (waiters, available, leased, clean) = state.close() { + XCTAssertTrue(waiters.isEmpty) + XCTAssertTrue(available.isEmpty) + XCTAssertTrue(leased.isEmpty) + XCTAssertFalse(clean) + } else { + XCTFail("Expecting snapshot") + } + + let connection = Connection(channel: EmbeddedChannel(), provider: self.http1ConnectionProvider) + + action = state.offer(connection: connection) + guard case .closeAnd(_, .closeProvider) = action else { + XCTFail("unexpected action \(action)") + return + } + + connectionPromise.fail(TempError()) + setupPromise.succeed(()) + } + + func testShutdownOnPendingAndError() { + var state = HTTP1ConnectionProvider.ConnectionsState(eventLoop: self.eventLoop) + + XCTAssertTrue(state.enqueue()) + + let connectionPromise = self.eventLoop.makePromise(of: Connection.self) + let setupPromise = self.eventLoop.makePromise(of: Void.self) + let waiter = HTTP1ConnectionProvider.Waiter(promise: connectionPromise, setupComplete: setupPromise.futureResult, preference: .indifferent) + var action = state.acquire(waiter: waiter) + + guard case .create = action else { + XCTFail("unexpected action \(action)") + return + } + + let snapshot = state.testsOnly_getInternalState() + XCTAssertEqual(snapshot.openedConnectionsCount, 1) + + if let (waiters, available, leased, clean) = state.close() { + XCTAssertTrue(waiters.isEmpty) + XCTAssertTrue(available.isEmpty) + XCTAssertTrue(leased.isEmpty) + XCTAssertFalse(clean) + } else { + XCTFail("Expecting snapshot") + } + + action = state.connectFailed() + guard case .closeProvider = action else { + XCTFail("unexpected action \(action)") + return + } + + connectionPromise.fail(TempError()) + setupPromise.succeed(()) + } + + func testShutdownTimeout() { + var state = HTTP1ConnectionProvider.ConnectionsState(eventLoop: self.eventLoop) + + var snapshot = self.http1ConnectionProvider.state.testsOnly_getInternalState() + + let connection = Connection(channel: EmbeddedChannel(), provider: self.http1ConnectionProvider) + snapshot.availableConnections.append(connection) + snapshot.openedConnectionsCount = 1 + + state.testsOnly_setInternalState(snapshot) + + if let (waiters, available, leased, clean) = state.close() { + XCTAssertTrue(waiters.isEmpty) + XCTAssertFalse(available.isEmpty) + XCTAssertTrue(leased.isEmpty) + XCTAssertTrue(clean) + } else { + XCTFail("Expecting snapshot") + } + + let action = state.timeout(connection: connection) + switch action { + case .closeAnd(_, let next): + switch next { + case .closeProvider: + // expected + break + default: + XCTFail("Unexpected action: \(action)") + } + default: + XCTFail("Unexpected action: \(action)") + } + } + + func testShutdownRemoteClosed() { + var state = HTTP1ConnectionProvider.ConnectionsState(eventLoop: self.eventLoop) + + var snapshot = self.http1ConnectionProvider.state.testsOnly_getInternalState() + + let connection = Connection(channel: EmbeddedChannel(), provider: self.http1ConnectionProvider) + snapshot.availableConnections.append(connection) + snapshot.openedConnectionsCount = 1 + + state.testsOnly_setInternalState(snapshot) + + if let (waiters, available, leased, clean) = state.close() { + XCTAssertTrue(waiters.isEmpty) + XCTAssertFalse(available.isEmpty) + XCTAssertTrue(leased.isEmpty) + XCTAssertTrue(clean) + } else { + XCTFail("Expecting snapshot") + } + + let action = state.remoteClosed(connection: connection) + switch action { + case .closeProvider: + // expected + break + default: + XCTFail("Unexpected action: \(action)") + } + } + + // MARK: - Helpers + override func setUp() { XCTAssertNil(self.eventLoop) XCTAssertNil(self.http1ConnectionProvider) From 9f65902ec6212eff8492e2c5f677d39c81a7f402 Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Mon, 22 Jun 2020 16:19:16 +0100 Subject: [PATCH 07/12] fix missing connect timeout and make tests safer (#267) * fix missing connect timeout and make tests safer * swiftformat and linux tests * fix timeout test * speedup another test * make tests safer --- Sources/AsyncHTTPClient/Utils.swift | 7 +++++- .../HTTPClientNIOTSTests.swift | 23 ++++++++----------- .../HTTPClientTests+XCTest.swift | 1 + .../HTTPClientTests.swift | 16 ++++++++++++- 4 files changed, 31 insertions(+), 16 deletions(-) diff --git a/Sources/AsyncHTTPClient/Utils.swift b/Sources/AsyncHTTPClient/Utils.swift index 7da957b07..dd30e0393 100644 --- a/Sources/AsyncHTTPClient/Utils.swift +++ b/Sources/AsyncHTTPClient/Utils.swift @@ -79,7 +79,7 @@ extension NIOClientTCPBootstrap { requiresTLS: Bool, configuration: HTTPClient.Configuration ) throws -> NIOClientTCPBootstrap { - let bootstrap: NIOClientTCPBootstrap + var bootstrap: NIOClientTCPBootstrap #if canImport(Network) // if eventLoop is compatible with NIOTransportServices create a NIOTSConnectionBootstrap if #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *), let tsBootstrap = NIOTSConnectionBootstrap(validatingGroup: eventLoop) { @@ -106,10 +106,15 @@ extension NIOClientTCPBootstrap { } #endif + if let timeout = configuration.timeout.connect { + bootstrap = bootstrap.connectTimeout(timeout) + } + // don't enable TLS if we have a proxy, this will be enabled later on if requiresTLS, configuration.proxy == nil { return bootstrap.enableTLS() } + return bootstrap } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift index d37ecae2a..ce71c5fab 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift @@ -74,19 +74,19 @@ class HTTPClientNIOTSTests: XCTestCase { func testConnectionFailError() { guard isTestingNIOTS() else { return } let httpBin = HTTPBin(ssl: true) - let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup)) + let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), + configuration: .init(timeout: .init(connect: .milliseconds(100), + read: .milliseconds(100)))) + defer { XCTAssertNoThrow(try httpClient.syncShutdown(requiresCleanClose: true)) } + let port = httpBin.port XCTAssertNoThrow(try httpBin.shutdown()) - do { - _ = try httpClient.get(url: "https://localhost:\(port)/get").wait() - XCTFail("This should have failed") - } catch ChannelError.connectTimeout { - } catch { - XCTFail("Error should have been ChannelError.connectTimeout not \(type(of: error))") + XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(port)/get").wait()) { error in + XCTAssertEqual(.connectTimeout(.milliseconds(100)), error as? ChannelError) } } @@ -103,13 +103,8 @@ class HTTPClientNIOTSTests: XCTestCase { XCTAssertNoThrow(try httpBin.shutdown()) } - do { - _ = try httpClient.get(url: "https://localhost:\(httpBin.port)/get").wait() - XCTFail("This should have failed") - } catch let error as HTTPClient.NWTLSError { - XCTAssertEqual(error.status, errSSLHandshakeFail) - } catch { - XCTFail("Error should have been NWTLSError not \(type(of: error))") + XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(httpBin.port)/get").wait()) { error in + XCTAssertEqual((error as? HTTPClient.NWTLSError)?.status, errSSLHandshakeFail) } #endif } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 7e64e0a8e..9c824a885 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -46,6 +46,7 @@ extension HTTPClientTests { ("testStreaming", testStreaming), ("testRemoteClose", testRemoteClose), ("testReadTimeout", testReadTimeout), + ("testConnectTimeout", testConnectTimeout), ("testDeadline", testDeadline), ("testCancel", testCancel), ("testStressCancel", testStressCancel), diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 4263863aa..dcaddf5b1 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -422,6 +422,20 @@ class HTTPClientTests: XCTestCase { } } + func testConnectTimeout() throws { + let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), + configuration: .init(timeout: .init(connect: .milliseconds(100), read: .milliseconds(150)))) + + defer { + XCTAssertNoThrow(try httpClient.syncShutdown()) + } + + // This must throw as 198.51.100.254 is reserved for documentation only + XCTAssertThrowsError(try httpClient.get(url: "http://198.51.100.254:65535/get").wait()) { error in + XCTAssertEqual(.connectTimeout(.milliseconds(100)), error as? ChannelError) + } + } + func testDeadline() throws { XCTAssertThrowsError(try self.defaultClient.get(url: self.defaultHTTPBinURLPrefix + "wait", deadline: .now() + .milliseconds(150)).wait(), "Should fail") { error in guard case let error = error as? HTTPClientError, error == .readTimeout else { @@ -1661,7 +1675,7 @@ class HTTPClientTests: XCTestCase { } func testAvoidLeakingTLSHandshakeCompletionPromise() { - let localClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup)) + let localClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), configuration: .init(timeout: .init(connect: .milliseconds(100)))) let localHTTPBin = HTTPBin() let port = localHTTPBin.port XCTAssertNoThrow(try localHTTPBin.shutdown()) From f6f99d2064aaf2ab2aa9217adc733422bd314532 Mon Sep 17 00:00:00 2001 From: Max Desiatov Date: Mon, 22 Jun 2020 17:39:58 +0100 Subject: [PATCH 08/12] Fix doc comment for redirectConfiguration (#266) `redirectConfiguration` can't default to `false` as it's not a boolean value, and the default value is `RedirectConfiguration()`. Co-authored-by: Johannes Weiss Co-authored-by: Artem Redkin --- Sources/AsyncHTTPClient/HTTPClient.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 26f5321b8..95826dd75 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -621,7 +621,7 @@ public class HTTPClient { public struct Configuration { /// TLS configuration, defaults to `TLSConfiguration.forClient()`. public var tlsConfiguration: Optional - /// Enables following 3xx redirects automatically, defaults to `false`. + /// Enables following 3xx redirects automatically, defaults to `RedirectConfiguration()`. /// /// Following redirects are supported: /// - `301: Moved Permanently` From 233f18b5419f839aa10f47943c1e547c39cc3429 Mon Sep 17 00:00:00 2001 From: Dimitri Bouniol Date: Mon, 22 Jun 2020 09:56:01 -0700 Subject: [PATCH 09/12] Fixed an issue where redirects to socket path-based servers from any server was always allowed (#259) * Added tests that ensure redirects to unix socket paths from a regular HTTP server are disallowed. Motivation: Currently, redirects to any supported URL scheme will always be allowed, despite code being in place to seemingly prevent it. See #230. Modifications: - Added a method to HTTPBin to redirect to the specified target. - Added failing tests that perform redirects from a regular server to a socket-based server and vice versa. Result: Failing tests that show that the existing redirect checks were inadequate. * Fixed an issue where redirects to socket path-based servers from any server was always allowed. Motivation: An arbitrary HTTP(S) server should not be able to trigger redirects, and thus activity, to a local socket-path based server, though the opposite may be a valid scenario. Currently, requests in either direction are allowed since the checks don't actually check the destination scheme. Modifications: - Refactored `hostSchemes`/`unixSchemes` to `hostRestrictedSchemes`/`allSupportedSchemes`, which better describes what they do. - Refactored `Request.supports()` to `Request.supportsRedirects(to:)` since it is only used by Redirects now. - Check the destination URL's scheme rather than the current URL's scheme when validating a redirect. Result: Closes #230 Co-authored-by: Artem Redkin --- Sources/AsyncHTTPClient/HTTPHandler.swift | 14 +-- .../HTTPClientTestUtils.swift | 6 ++ .../HTTPClientTests.swift | 89 +++++++++++++++++++ 3 files changed, 103 insertions(+), 6 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index d152e6660..41867ac3a 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -107,8 +107,8 @@ extension HTTPClient { /// UNIX Domain Socket HTTP request. case unixSocket(_ scheme: UnixScheme) - private static var hostSchemes = ["http", "https"] - private static var unixSchemes = ["unix", "http+unix", "https+unix"] + private static var hostRestrictedSchemes: Set = ["http", "https"] + private static var allSupportedSchemes: Set = ["http", "https", "unix", "http+unix", "https+unix"] init(forScheme scheme: String) throws { switch scheme { @@ -158,12 +158,14 @@ extension HTTPClient { } } - func supports(scheme: String) -> Bool { + func supportsRedirects(to scheme: String?) -> Bool { + guard let scheme = scheme?.lowercased() else { return false } + switch self { case .host: - return Kind.hostSchemes.contains(scheme) + return Kind.hostRestrictedSchemes.contains(scheme) case .unixSocket: - return Kind.unixSchemes.contains(scheme) + return Kind.allSupportedSchemes.contains(scheme) } } } @@ -1051,7 +1053,7 @@ internal struct RedirectHandler { return nil } - guard self.request.kind.supports(scheme: self.request.scheme) else { + guard self.request.kind.supportsRedirects(to: url.scheme) else { return nil } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index e5df9cfc5..6beba8938 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -495,6 +495,12 @@ internal final class HttpBinHandler: ChannelInboundHandler { headers.add(name: "Location", value: "/redirect/infinite1") self.resps.append(HTTPResponseBuilder(status: .found, headers: headers)) return + case "/redirect/target": + var headers = self.responseHeaders + let targetURL = req.headers["X-Target-Redirect-URL"].first ?? "" + headers.add(name: "Location", value: targetURL) + self.resps.append(HTTPResponseBuilder(status: .found, headers: headers)) + return case "/percent%20encoded": if req.method != .GET { self.resps.append(HTTPResponseBuilder(status: .methodNotAllowed)) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index dcaddf5b1..103580116 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -347,6 +347,95 @@ class HTTPClientTests: XCTestCase { response = try localClient.get(url: self.defaultHTTPBinURLPrefix + "redirect/https?port=\(httpsBin.port)").wait() XCTAssertEqual(response.status, .ok) + + XCTAssertNoThrow(try TemporaryFileHelpers.withTemporaryUnixDomainSocketPathName { httpSocketPath in + XCTAssertNoThrow(try TemporaryFileHelpers.withTemporaryUnixDomainSocketPathName { httpsSocketPath in + let socketHTTPBin = HTTPBin(bindTarget: .unixDomainSocket(httpSocketPath)) + let socketHTTPSBin = HTTPBin(ssl: true, bindTarget: .unixDomainSocket(httpsSocketPath)) + defer { + XCTAssertNoThrow(try socketHTTPBin.shutdown()) + XCTAssertNoThrow(try socketHTTPSBin.shutdown()) + } + + // From HTTP or HTTPS to HTTP+UNIX should fail to redirect + var targetURL = "http+unix://\(httpSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/ok" + var request = try Request(url: self.defaultHTTPBinURLPrefix + "redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + var response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .found) + XCTAssertEqual(response.headers.first(name: "Location"), targetURL) + + request = try Request(url: "https://localhost:\(httpsBin.port)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .found) + XCTAssertEqual(response.headers.first(name: "Location"), targetURL) + + // From HTTP or HTTPS to HTTPS+UNIX should also fail to redirect + targetURL = "https+unix://\(httpsSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/ok" + request = try Request(url: self.defaultHTTPBinURLPrefix + "redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .found) + XCTAssertEqual(response.headers.first(name: "Location"), targetURL) + + request = try Request(url: "https://localhost:\(httpsBin.port)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .found) + XCTAssertEqual(response.headers.first(name: "Location"), targetURL) + + // ... while HTTP+UNIX to HTTP, HTTPS, or HTTP(S)+UNIX should succeed + targetURL = self.defaultHTTPBinURLPrefix + "ok" + request = try Request(url: "http+unix://\(httpSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .ok) + + targetURL = "https://localhost:\(httpsBin.port)/ok" + request = try Request(url: "http+unix://\(httpSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .ok) + + targetURL = "http+unix://\(httpSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/ok" + request = try Request(url: "http+unix://\(httpSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .ok) + + targetURL = "https+unix://\(httpsSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/ok" + request = try Request(url: "http+unix://\(httpSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .ok) + + // ... and HTTPS+UNIX to HTTP, HTTPS, or HTTP(S)+UNIX should succeed + targetURL = self.defaultHTTPBinURLPrefix + "ok" + request = try Request(url: "https+unix://\(httpsSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .ok) + + targetURL = "https://localhost:\(httpsBin.port)/ok" + request = try Request(url: "https+unix://\(httpsSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .ok) + + targetURL = "http+unix://\(httpSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/ok" + request = try Request(url: "https+unix://\(httpsSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .ok) + + targetURL = "https+unix://\(httpsSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/ok" + request = try Request(url: "https+unix://\(httpsSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .ok) + }) + }) } func testHttpHostRedirect() { From 29dc2c79d8d8cc31ea3325e86d309b1359eab7b3 Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Tue, 23 Jun 2020 15:00:19 +0100 Subject: [PATCH 10/12] notify delegate about connect errors (#245) --- Sources/AsyncHTTPClient/ConnectionPool.swift | 8 ++-- Sources/AsyncHTTPClient/HTTPClient.swift | 21 ++++++---- .../HTTPClientInternalTests+XCTest.swift | 1 + .../HTTPClientInternalTests.swift | 39 +++++++++++++++++++ .../HTTPClientTests+XCTest.swift | 1 + .../HTTPClientTests.swift | 27 +++++++++++++ .../RequestValidationTests+XCTest.swift | 8 ++++ 7 files changed, 94 insertions(+), 11 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool.swift b/Sources/AsyncHTTPClient/ConnectionPool.swift index ce648469c..5e44da8a9 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool.swift @@ -513,13 +513,13 @@ class HTTP1ConnectionProvider { } private func makeChannel(preference: HTTPClient.EventLoopPreference) -> EventLoopFuture { - let eventLoop = preference.bestEventLoop ?? self.eventLoop + let channelEventLoop = preference.bestEventLoop ?? self.eventLoop let requiresTLS = self.key.scheme.requiresTLS let bootstrap: NIOClientTCPBootstrap do { - bootstrap = try NIOClientTCPBootstrap.makeHTTPClientBootstrapBase(on: eventLoop, host: self.key.host, port: self.key.port, requiresTLS: requiresTLS, configuration: self.configuration) + bootstrap = try NIOClientTCPBootstrap.makeHTTPClientBootstrapBase(on: channelEventLoop, host: self.key.host, port: self.key.port, requiresTLS: requiresTLS, configuration: self.configuration) } catch { - return eventLoop.makeFailedFuture(error) + return channelEventLoop.makeFailedFuture(error) } let channel: EventLoopFuture @@ -564,7 +564,7 @@ class HTTP1ConnectionProvider { error = HTTPClient.NWErrorHandler.translateError(error) } #endif - return self.eventLoop.makeFailedFuture(error) + return channelEventLoop.makeFailedFuture(error) } } diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 95826dd75..5fee5e0a1 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -545,6 +545,14 @@ public class HTTPClient { deadline: deadline, setupComplete: setupComplete.futureResult, logger: logger) + + let taskHandler = TaskHandler(task: task, + kind: request.kind, + delegate: delegate, + redirectHandler: redirectHandler, + ignoreUncleanSSLShutdown: self.configuration.ignoreUncleanSSLShutdown, + logger: logger) + connection.flatMap { connection -> EventLoopFuture in logger.debug("got connection for request", metadata: ["ahc-connection": "\(connection)", @@ -561,12 +569,6 @@ public class HTTPClient { } return future.flatMap { - let taskHandler = TaskHandler(task: task, - kind: request.kind, - delegate: delegate, - redirectHandler: redirectHandler, - ignoreUncleanSSLShutdown: self.configuration.ignoreUncleanSSLShutdown, - logger: logger) return channel.pipeline.addHandler(taskHandler) }.flatMap { task.setConnection(connection) @@ -590,7 +592,12 @@ public class HTTPClient { } }.always { _ in setupComplete.succeed(()) - }.cascadeFailure(to: task.promise) + }.whenFailure { error in + taskHandler.callOutToDelegateFireAndForget { task in + delegate.didReceiveError(task: task, error) + } + task.promise.fail(error) + } return task } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests+XCTest.swift index 8a89eb740..14c830e51 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests+XCTest.swift @@ -43,6 +43,7 @@ extension HTTPClientInternalTests { ("testUploadStreamingIsCalledOnTaskEL", testUploadStreamingIsCalledOnTaskEL), ("testWeCanActuallyExactlySetTheEventLoops", testWeCanActuallyExactlySetTheEventLoops), ("testTaskPromiseBoundToEL", testTaskPromiseBoundToEL), + ("testConnectErrorCalloutOnCorrectEL", testConnectErrorCalloutOnCorrectEL), ("testInternalRequestURI", testInternalRequestURI), ] } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift index 39d0c6f3a..34891c632 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift @@ -960,6 +960,45 @@ class HTTPClientInternalTests: XCTestCase { XCTAssertNoThrow(try task.wait()) } + func testConnectErrorCalloutOnCorrectEL() throws { + class TestDelegate: HTTPClientResponseDelegate { + typealias Response = Void + + let expectedEL: EventLoop + var receivedError: Bool = false + + init(expectedEL: EventLoop) { + self.expectedEL = expectedEL + } + + func didFinishRequest(task: HTTPClient.Task) throws {} + + func didReceiveError(task: HTTPClient.Task, _ error: Error) { + self.receivedError = true + XCTAssertTrue(self.expectedEL.inEventLoop) + } + } + + let elg = getDefaultEventLoopGroup(numberOfThreads: 2) + let el1 = elg.next() + let el2 = elg.next() + + let httpBin = HTTPBin(refusesConnections: true) + let client = HTTPClient(eventLoopGroupProvider: .shared(elg)) + + defer { + XCTAssertNoThrow(try client.syncShutdown()) + XCTAssertNoThrow(try elg.syncShutdownGracefully()) + } + + let request = try HTTPClient.Request(url: "http://localhost:\(httpBin.port)/get") + let delegate = TestDelegate(expectedEL: el1) + XCTAssertNoThrow(try httpBin.shutdown()) + let task = client.execute(request: request, delegate: delegate, eventLoop: .init(.testOnly_exact(channelOn: el2, delegateOn: el1))) + XCTAssertThrowsError(try task.wait()) + XCTAssertTrue(delegate.receivedError) + } + func testInternalRequestURI() throws { let request1 = try Request(url: "https://someserver.com:8888/some/path?foo=bar") XCTAssertEqual(request1.kind, .host) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 9c824a885..5fe7b2417 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -115,6 +115,7 @@ extension HTTPClientTests { ("testAllMethodsLog", testAllMethodsLog), ("testClosingIdleConnectionsInPoolLogsInTheBackground", testClosingIdleConnectionsInPoolLogsInTheBackground), ("testUploadStreamingNoLength", testUploadStreamingNoLength), + ("testConnectErrorPropagatedToDelegate", testConnectErrorPropagatedToDelegate), ("testDelegateCallinsTolerateRandomEL", testDelegateCallinsTolerateRandomEL), ("testContentLengthTooLongFails", testContentLengthTooLongFails), ("testContentLengthTooShortFails", testContentLengthTooShortFails), diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 103580116..df144b70b 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2405,6 +2405,33 @@ class HTTPClientTests: XCTestCase { XCTAssertNoThrow(try future.wait()) } + func testConnectErrorPropagatedToDelegate() throws { + class TestDelegate: HTTPClientResponseDelegate { + typealias Response = Void + var error: Error? + func didFinishRequest(task: HTTPClient.Task) throws {} + func didReceiveError(task: HTTPClient.Task, _ error: Error) { + self.error = error + } + } + + let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), + configuration: .init(timeout: .init(connect: .milliseconds(10)))) + + defer { + XCTAssertNoThrow(try httpClient.syncShutdown()) + } + + // This must throw as 198.51.100.254 is reserved for documentation only + let request = try HTTPClient.Request(url: "http://198.51.100.254:65535/get") + let delegate = TestDelegate() + + XCTAssertThrowsError(try httpClient.execute(request: request, delegate: delegate).wait()) { error in + XCTAssertEqual(.connectTimeout(.milliseconds(10)), error as? ChannelError) + XCTAssertEqual(.connectTimeout(.milliseconds(10)), delegate.error as? ChannelError) + } + } + func testDelegateCallinsTolerateRandomEL() throws { class TestDelegate: HTTPClientResponseDelegate { typealias Response = Void diff --git a/Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift b/Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift index 1b0fa7814..08d4cfb32 100644 --- a/Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift @@ -32,6 +32,14 @@ extension RequestValidationTests { ("testGET_HEAD_DELETE_CONNECTRequestCanHaveBody", testGET_HEAD_DELETE_CONNECTRequestCanHaveBody), ("testInvalidHeaderFieldNames", testInvalidHeaderFieldNames), ("testValidHeaderFieldNames", testValidHeaderFieldNames), + ("testNoHeadersNoBody", testNoHeadersNoBody), + ("testNoHeadersHasBody", testNoHeadersHasBody), + ("testContentLengthHeaderNoBody", testContentLengthHeaderNoBody), + ("testContentLengthHeaderHasBody", testContentLengthHeaderHasBody), + ("testTransferEncodingHeaderNoBody", testTransferEncodingHeaderNoBody), + ("testTransferEncodingHeaderHasBody", testTransferEncodingHeaderHasBody), + ("testBothHeadersNoBody", testBothHeadersNoBody), + ("testBothHeadersHasBody", testBothHeadersHasBody), ] } } From ac8b43372022dedb1da6e57a9adebcc34df82486 Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Tue, 23 Jun 2020 15:11:57 +0100 Subject: [PATCH 11/12] make all tests pass --- .../RequestValidationTests.swift | 53 ++++++++++--------- 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/RequestValidationTests.swift b/Tests/AsyncHTTPClientTests/RequestValidationTests.swift index edf0bb736..652483293 100644 --- a/Tests/AsyncHTTPClientTests/RequestValidationTests.swift +++ b/Tests/AsyncHTTPClientTests/RequestValidationTests.swift @@ -92,7 +92,7 @@ class RequestValidationTests: XCTestCase { // Method kind User sets Body Expectation // ---------------------------------------------------------------------------------- // .GET, .HEAD, .DELETE, .CONNECT, .TRACE nothing nil Neither CL nor chunked - // other nothing nil chunked + // other nothing nil CL=0 func testNoHeadersNoBody() throws { for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT, .TRACE] { var headers: HTTPHeaders = .init() @@ -104,46 +104,50 @@ class RequestValidationTests: XCTestCase { for method: HTTPMethod in [.POST, .PUT] { var headers: HTTPHeaders = .init() XCTAssertNoThrow(try headers.validate(method: method, body: nil)) - // TODO: This should be CL=0 ??? https://tools.ietf.org/html/rfc7230#section-3.3.2 - XCTAssertTrue(headers["content-length"].isEmpty) - XCTAssertTrue(headers["transfer-encoding"].contains("chunked")) + XCTAssertEqual(headers["content-length"].first, "0") + XCTAssertFalse(headers["transfer-encoding"].contains("chunked")) } } // Method kind User sets Body Expectation // -------------------------------------------------------------------------------------- - // .GET, .HEAD, .DELETE, .CONNECT, .TRACE nothing not nil Neither CL nor chunked - // other nothing not nil chunked + // .GET, .HEAD, .DELETE, .CONNECT, .TRACE nothing not nil CL or chunked + // other nothing not nil CL or chunked func testNoHeadersHasBody() throws { + // Body length is known for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT] { var headers: HTTPHeaders = .init() XCTAssertNoThrow(try headers.validate(method: method, body: .byteBuffer(ByteBuffer(bytes: [0])))) + XCTAssertEqual(headers["content-length"].first, "1") + XCTAssertTrue(headers["transfer-encoding"].isEmpty) + } - // TODO: Logically, this should be a Content-Length: 1 - + // Body length is _not_ known + for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT] { + var headers: HTTPHeaders = .init() + let body: HTTPClient.Body = .stream { writer in + writer.write(.byteBuffer(ByteBuffer(bytes: [0]))) + } + XCTAssertNoThrow(try headers.validate(method: method, body: body)) XCTAssertTrue(headers["content-length"].isEmpty) - XCTAssertTrue(headers["transfer-encoding"].isEmpty) + XCTAssertTrue(headers["transfer-encoding"].contains("chunked")) } + // Body length is known for method: HTTPMethod in [.POST, .PUT] { var headers: HTTPHeaders = .init() XCTAssertNoThrow(try headers.validate(method: method, body: .byteBuffer(ByteBuffer(bytes: [0])))) - - // TODO: Logically, this should be a Content-Length: 1 since we know size, or chunked if we don't - - XCTAssertTrue(headers["content-length"].isEmpty) - XCTAssertTrue(headers["transfer-encoding"].contains("chunked")) + XCTAssertEqual(headers["content-length"].first, "1") + XCTAssertTrue(headers["transfer-encoding"].isEmpty) } + // Body length is _not_ known for method: HTTPMethod in [.POST, .PUT] { var headers: HTTPHeaders = .init() let body: HTTPClient.Body = .stream { writer in writer.write(.byteBuffer(ByteBuffer(bytes: [0]))) } XCTAssertNoThrow(try headers.validate(method: method, body: body)) - - // TODO: Logically, this should be a Content-Length: 1 since we know size, or chunked if we don't - XCTAssertTrue(headers["content-length"].isEmpty) XCTAssertTrue(headers["transfer-encoding"].contains("chunked")) } @@ -151,14 +155,13 @@ class RequestValidationTests: XCTestCase { // Method kind User sets Body Expectation // ------------------------------------------------------------------------------ - // .GET, .HEAD, .DELETE, .CONNECT, .TRACE content-length nil CL=0 + // .GET, .HEAD, .DELETE, .CONNECT, .TRACE content-length nil Neither CL nor chunked // other content-length nil CL=0 func testContentLengthHeaderNoBody() throws { for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT, .TRACE] { var headers: HTTPHeaders = .init([("Content-Length", "1")]) XCTAssertNoThrow(try headers.validate(method: method, body: nil)) - // TODO: this should be Neither CL nor chunked https://tools.ietf.org/html/rfc7230#section-3.3.2 - XCTAssertEqual(headers["content-length"].first, "0") + XCTAssertTrue(headers["content-length"].isEmpty) XCTAssertTrue(headers["transfer-encoding"].isEmpty) } @@ -192,21 +195,21 @@ class RequestValidationTests: XCTestCase { // Method kind User sets Body Expectation // ------------------------------------------------------------------------------------------ - // .GET, .HEAD, .DELETE, .CONNECT, .TRACE transfer-encoding: chunked nil chunked - // other transfer-encoding: chunked nil chunked + // .GET, .HEAD, .DELETE, .CONNECT, .TRACE transfer-encoding: chunked nil nil + // other transfer-encoding: chunked nil nil func testTransferEncodingHeaderNoBody() throws { for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT, .TRACE] { var headers: HTTPHeaders = .init([("Transfer-Encoding", "chunked")]) XCTAssertNoThrow(try headers.validate(method: method, body: nil)) XCTAssertTrue(headers["content-length"].isEmpty) - XCTAssertTrue(headers["transfer-encoding"].contains("chunked")) + XCTAssertFalse(headers["transfer-encoding"].contains("chunked")) } for method: HTTPMethod in [.POST, .PUT] { var headers: HTTPHeaders = .init([("Transfer-Encoding", "chunked")]) XCTAssertNoThrow(try headers.validate(method: method, body: nil)) - XCTAssertTrue(headers["content-length"].isEmpty) - XCTAssertTrue(headers["transfer-encoding"].contains("chunked")) + XCTAssertEqual(headers["content-length"].first, "0") + XCTAssertFalse(headers["transfer-encoding"].contains("chunked")) } } From 08c7a33f475b205e2aa53b4f0cb4e445a2b31d6c Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Tue, 23 Jun 2020 15:22:25 +0100 Subject: [PATCH 12/12] swiftformat --- Sources/AsyncHTTPClient/RequestValidation.swift | 2 +- Tests/AsyncHTTPClientTests/RequestValidationTests.swift | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Sources/AsyncHTTPClient/RequestValidation.swift b/Sources/AsyncHTTPClient/RequestValidation.swift index 6e7447cd9..a9c1678f4 100644 --- a/Sources/AsyncHTTPClient/RequestValidation.swift +++ b/Sources/AsyncHTTPClient/RequestValidation.swift @@ -18,7 +18,7 @@ import NIOHTTP1 extension HTTPHeaders { mutating func validate(method: HTTPMethod, body: HTTPClient.Body?) throws { // validate transfer encoding and content length (https://tools.ietf.org/html/rfc7230#section-3.3.1) - if self.contains(name: "Transfer-Encoding") && self.contains(name: "Content-Length") { + if self.contains(name: "Transfer-Encoding"), self.contains(name: "Content-Length") { throw HTTPClientError.incompatibleHeaders } diff --git a/Tests/AsyncHTTPClientTests/RequestValidationTests.swift b/Tests/AsyncHTTPClientTests/RequestValidationTests.swift index 652483293..3e34f905c 100644 --- a/Tests/AsyncHTTPClientTests/RequestValidationTests.swift +++ b/Tests/AsyncHTTPClientTests/RequestValidationTests.swift @@ -173,10 +173,10 @@ class RequestValidationTests: XCTestCase { } } - // Method kind User sets Body Expectation - // ---------------------------------------------------------------------------------- - // .GET, .HEAD, .DELETE, .CONNECT, .TRACE content-length not nil CL=1 - // other content-length nit nil CL=1 + // Method kind User sets Body Expectation + // -------------------------------------------------------------------------- + // .GET, .HEAD, .DELETE, .CONNECT content-length not nil CL=1 + // other content-length nit nil CL=1 func testContentLengthHeaderHasBody() throws { for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT] { var headers: HTTPHeaders = .init([("Content-Length", "1")])