Skip to content

refactor to make request non-throwing #56

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from
Closed

refactor to make request non-throwing #56

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
96f4114
refactor to make request non-throwing
artemredkin Jun 30, 2019
543e6a4
fix missing test on linux
artemredkin Jul 1, 2019
e1872a8
Merge branch 'master' into non_throwing_request_init
artemredkin Jul 2, 2019
e1ddb85
review fix: remove version field
artemredkin Jul 2, 2019
f9e2027
Merge branch 'master' into non_throwing_request_init
artemredkin Jul 3, 2019
c22aa55
Merge branch 'master' into non_throwing_request_init
weissi Jul 4, 2019
8e43a4d
Merge branch 'master' into non_throwing_request_init
artemredkin Jul 8, 2019
b733bbe
make non-throwing
artemredkin Jul 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 33 additions & 31 deletions Sources/AsyncHTTPClient/HTTPClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,57 +59,61 @@ public class HTTPClient {
}

public func get(url: String, deadline: NIODeadline? = nil) -> EventLoopFuture<Response> {
do {
let request = try Request(url: url, method: .GET)
return self.execute(request: request, deadline: deadline)
} catch {
return self.eventLoopGroup.next().makeFailedFuture(error)
guard let request = Request(url: url, method: .GET) else {
return self.eventLoopGroup.next().makeFailedFuture(HTTPClientError.invalidURL)
}
return self.execute(request: request, deadline: deadline)
}

public func post(url: String, body: Body? = nil, deadline: NIODeadline? = nil) -> EventLoopFuture<Response> {
do {
let request = try HTTPClient.Request(url: url, method: .POST, body: body)
return self.execute(request: request, deadline: deadline)
} catch {
return self.eventLoopGroup.next().makeFailedFuture(error)
guard let request = Request(url: url, method: .POST, body: body) else {
return self.eventLoopGroup.next().makeFailedFuture(HTTPClientError.invalidURL)
}
return self.execute(request: request, deadline: deadline)
}

public func patch(url: String, body: Body? = nil, deadline: NIODeadline? = nil) -> EventLoopFuture<Response> {
do {
let request = try HTTPClient.Request(url: url, method: .PATCH, body: body)
return self.execute(request: request, deadline: deadline)
} catch {
return self.eventLoopGroup.next().makeFailedFuture(error)
guard let request = HTTPClient.Request(url: url, method: .PATCH, body: body) else {
return self.eventLoopGroup.next().makeFailedFuture(HTTPClientError.invalidURL)
}
return self.execute(request: request, deadline: deadline)
}

public func put(url: String, body: Body? = nil, deadline: NIODeadline? = nil) -> EventLoopFuture<Response> {
do {
let request = try HTTPClient.Request(url: url, method: .PUT, body: body)
return self.execute(request: request, deadline: deadline)
} catch {
return self.eventLoopGroup.next().makeFailedFuture(error)
guard let request = HTTPClient.Request(url: url, method: .PUT, body: body) else {
return self.eventLoopGroup.next().makeFailedFuture(HTTPClientError.invalidURL)
}
return self.execute(request: request, deadline: deadline)
}

public func delete(url: String, deadline: NIODeadline? = nil) -> EventLoopFuture<Response> {
guard let request = Request(url: url, method: .DELETE) else {
return self.eventLoopGroup.next().makeFailedFuture(HTTPClientError.invalidURL)
}
return self.execute(request: request, deadline: deadline)
}

public func execute(request: Request, deadline: NIODeadline? = nil) -> EventLoopFuture<Response> {
do {
let request = try Request(url: url, method: .DELETE)
return self.execute(request: request, deadline: deadline)
let requestWithHost = try RequestWithHost(request: request)
let accumulator = ResponseAccumulator(host: requestWithHost.host)
return self.execute(request: requestWithHost, delegate: accumulator, deadline: deadline).futureResult
} catch {
return self.eventLoopGroup.next().makeFailedFuture(error)
}
}

public func execute(request: Request, deadline: NIODeadline? = nil) -> EventLoopFuture<Response> {
let accumulator = ResponseAccumulator(request: request)
return self.execute(request: request, delegate: accumulator, deadline: deadline).futureResult
public func execute<T: HTTPClientResponseDelegate>(request: Request, delegate: T, deadline: NIODeadline? = nil) -> Task<T.Response> {
do {
return try self.execute(request: RequestWithHost(request: request), delegate: delegate, deadline: deadline)
} catch {
return Task(eventLoop: self.eventLoopGroup.next(), error: error)
}
}

public func execute<T: HTTPClientResponseDelegate>(request: Request, delegate: T, deadline: NIODeadline? = nil) -> Task<T.Response> {
private func execute<T: HTTPClientResponseDelegate>(request: RequestWithHost, delegate: T, deadline: NIODeadline? = nil) -> Task<T.Response> {
let eventLoop = self.eventLoopGroup.next()
let task = Task<T.Response>(eventLoop: eventLoop)

let redirectHandler: RedirectHandler<T.Response>?
if self.configuration.followRedirects {
Expand All @@ -120,8 +124,6 @@ public class HTTPClient {
redirectHandler = nil
}

let task = Task<T.Response>(eventLoop: eventLoop)

var bootstrap = ClientBootstrap(group: eventLoop)
.channelOption(ChannelOptions.socket(SocketOptionLevel(IPPROTO_TCP), TCP_NODELAY), value: 1)
.channelInitializer { channel in
Expand Down Expand Up @@ -178,7 +180,7 @@ public class HTTPClient {
}
}

private func resolveAddress(request: Request, proxy: Proxy?) -> (host: String, port: Int) {
private func resolveAddress(request: RequestWithHost, proxy: Proxy?) -> (host: String, port: Int) {
switch self.configuration.proxy {
case .none:
return (request.host, request.port)
Expand Down Expand Up @@ -225,7 +227,7 @@ public class HTTPClient {
}

private extension ChannelPipeline {
func addProxyHandler(for request: HTTPClient.Request, decoder: ByteToMessageHandler<HTTPResponseDecoder>, encoder: HTTPRequestEncoder, tlsConfiguration: TLSConfiguration?) -> EventLoopFuture<Void> {
func addProxyHandler(for request: HTTPClient.RequestWithHost, decoder: ByteToMessageHandler<HTTPResponseDecoder>, encoder: HTTPRequestEncoder, tlsConfiguration: TLSConfiguration?) -> EventLoopFuture<Void> {
let handler = HTTPClientProxyHandler(host: request.host, port: request.port, onConnect: { channel in
channel.pipeline.removeHandler(decoder).flatMap {
return channel.pipeline.addHandler(
Expand All @@ -239,7 +241,7 @@ private extension ChannelPipeline {
return self.addHandler(handler)
}

func addSSLHandlerIfNeeded(for request: HTTPClient.Request, tlsConfiguration: TLSConfiguration?) -> EventLoopFuture<Void> {
func addSSLHandlerIfNeeded(for request: HTTPClient.RequestWithHost, tlsConfiguration: TLSConfiguration?) -> EventLoopFuture<Void> {
guard request.useTLS else {
return self.eventLoop.makeSucceededFuture(())
}
Expand Down
113 changes: 78 additions & 35 deletions Sources/AsyncHTTPClient/HTTPHandler.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,49 +60,89 @@ extension HTTPClient {
}

public struct Request {
public var version: HTTPVersion
public var method: HTTPMethod
public var url: URL
public var scheme: String
public var host: String
public var headers: HTTPHeaders
public var body: Body?

public init(url: String, version: HTTPVersion = HTTPVersion(major: 1, minor: 1), method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil) throws {
public init?(url: String, method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil) {
guard let url = URL(string: url) else {
throw HTTPClientError.invalidURL
return nil
}

try self.init(url: url, version: version, method: method, headers: headers, body: body)
self.init(url: url, method: method, headers: headers, body: body)
}

public init(url: URL, version: HTTPVersion = HTTPVersion(major: 1, minor: 1), method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil) throws {
guard let scheme = url.scheme else {
public init(url: URL, method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil) {
self.method = method
self.url = url
self.headers = headers
self.body = body
}
}

struct RequestWithHost {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, I don't quite follow why this is necessary. I must be missing something... Couldn't we treat a Request's url property as the source of truth and control access to that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not about access control. URL's host property is optional, but we require it to be non-optional (so we know where to connect, what default domain to use in cookies and how to redirect using relative URL). Before that we would unwrap host (or throw) and then store in the request, but this leads to two downsides:

  1. We have to keep in sync host and url properties (and we cannot throw if we change a property, so url property would have to be set using a function, and this is a bit ugly, imho)
  2. creating request is a throwing init
    To address those two points I propose to introduce new internal type, sort of like "validated request", where user will not be able to change it and Request will not be throwing.
    Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation, I see now.

As we control the URL so we know its host property is set, can't we force unwrap instead of throwing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean control the URL? URL is passed in by the user, so we don't know what will be inside it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh. I'm sorry, I see now. So we swap the throwing initializer for a failed future from execute(). I guess that's a better tradeoff although it does mean that one initializer throws and the other doesn't.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. i prefer non-throwing APIs

  2. another option here is to define a custom url type in which host is not optional, and with [optional] initializers from string and URL

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

converted to optional. I'm not sure having our own URL type is the way to go atm, URL is used everywhere.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure having our own URL type is the way to go atm, URL is used everywhere.

@artemredkin the point was that instead of an internal RequestWithHost you can create and internal UrlWithHost (or whatever), as this is really what you want here, not a custom request object. this will help remove all the boilerplate wrapping eg L105-121

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But how one would pass it further? Handler needs access to URL, host, method and headers. If we don't want to expose UrlWithHost we will still need some wrapper to pass both Request and UrlWithHost to `TaskHandler. Does it make sense?

Copy link
Contributor

@tomerd tomerd Jul 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as i was working on demonstrating what i meant above, i believe i ended up with an even simpler and safer approach: #66

also fixes a bunch of other small bugs and removes runtime preconditions

wdyt?

var request: Request
var host: String

init(request: Request) throws {
guard let host = request.url.host else {
throw HTTPClientError.emptyHost
}

guard let scheme = request.url.scheme else {
throw HTTPClientError.emptyScheme
}

guard Request.isSchemeSupported(scheme: scheme) else {
guard RequestWithHost.isSchemeSupported(scheme: scheme) else {
throw HTTPClientError.unsupportedScheme(scheme)
}

guard let host = url.host else {
throw HTTPClientError.emptyHost
self.request = request
self.host = host
}

var method: HTTPMethod {
get {
return self.request.method
}
set {
self.request.method = newValue
}
}

var url: URL {
get {
return self.request.url
}
set {
self.request.url = newValue
}
}

self.version = version
self.method = method
self.url = url
self.scheme = scheme
self.host = host
self.headers = headers
self.body = body
var headers: HTTPHeaders {
get {
return self.request.headers
}
set {
self.request.headers = newValue
}
}

public var useTLS: Bool {
var body: Body? {
get {
return self.request.body
}
set {
self.request.body = newValue
}
}

var useTLS: Bool {
return self.url.scheme == "https"
}

public var port: Int {
var port: Int {
return self.url.port ?? (self.useTLS ? 443 : 80)
}

Expand Down Expand Up @@ -131,10 +171,10 @@ internal class ResponseAccumulator: HTTPClientResponseDelegate {
}

var state = State.idle
let request: HTTPClient.Request
let host: String

init(request: HTTPClient.Request) {
self.request = request
init(host: String) {
self.host = host
}

func didReceiveHead(task: HTTPClient.Task<Response>, _ head: HTTPResponseHead) -> EventLoopFuture<Void> {
Expand Down Expand Up @@ -180,9 +220,9 @@ internal class ResponseAccumulator: HTTPClientResponseDelegate {
case .idle:
preconditionFailure("no head received before end")
case .head(let head):
return Response(host: self.request.host, status: head.status, headers: head.headers, body: nil)
return Response(host: self.host, status: head.status, headers: head.headers, body: nil)
case .body(let head, let body):
return Response(host: self.request.host, status: head.status, headers: head.headers, body: body)
return Response(host: self.host, status: head.status, headers: head.headers, body: body)
case .end:
preconditionFailure("request already processed")
case .error(let error):
Expand Down Expand Up @@ -253,6 +293,11 @@ extension HTTPClient {
self.lock = Lock()
}

convenience init(eventLoop: EventLoop, error: Error) {
self.init(eventLoop: eventLoop)
self.fail(error)
}

public var futureResult: EventLoopFuture<Response> {
return self.promise.futureResult
}
Expand Down Expand Up @@ -291,7 +336,7 @@ extension HTTPClient {
internal struct TaskCancelEvent {}

internal class TaskHandler<T: HTTPClientResponseDelegate>: ChannelInboundHandler, ChannelOutboundHandler {
typealias OutboundIn = HTTPClient.Request
typealias OutboundIn = HTTPClient.RequestWithHost
typealias InboundIn = HTTPClientResponsePart
typealias OutboundOut = HTTPClientRequestPart

Expand Down Expand Up @@ -322,10 +367,10 @@ internal class TaskHandler<T: HTTPClientResponseDelegate>: ChannelInboundHandler
self.state = .idle
let request = unwrapOutboundIn(data)

var head = HTTPRequestHead(version: request.version, method: request.method, uri: request.url.uri)
var head = HTTPRequestHead(version: HTTPVersion(major: 1, minor: 1), method: request.method, uri: request.url.uri)
var headers = request.headers

if request.version.major == 1, request.version.minor == 1, !request.headers.contains(name: "Host") {
if !request.headers.contains(name: "Host") {
headers.add(name: "Host", value: request.host)
}

Expand All @@ -345,7 +390,7 @@ internal class TaskHandler<T: HTTPClientResponseDelegate>: ChannelInboundHandler
self.delegate.didSendRequestHead(task: self.task, head)
}

self.writeBody(request: request, context: context).whenComplete { result in
self.writeBody(request: request.request, context: context).whenComplete { result in
switch result {
case .success:
context.write(self.wrapOutboundOut(.end(nil)), promise: promise)
Expand Down Expand Up @@ -495,8 +540,8 @@ internal class TaskHandler<T: HTTPClientResponseDelegate>: ChannelInboundHandler
}

internal struct RedirectHandler<T> {
let request: HTTPClient.Request
let execute: (HTTPClient.Request) -> HTTPClient.Task<T>
let request: HTTPClient.RequestWithHost
let execute: (HTTPClient.RequestWithHost) -> HTTPClient.Task<T>

func redirectTarget(status: HTTPResponseStatus, headers: HTTPHeaders) -> URL? {
switch status {
Expand All @@ -514,7 +559,7 @@ internal struct RedirectHandler<T> {
return nil
}

guard HTTPClient.Request.isSchemeSupported(scheme: url.scheme) else {
guard HTTPClient.RequestWithHost.isSchemeSupported(scheme: url.scheme) else {
return nil
}

Expand All @@ -537,9 +582,7 @@ internal struct RedirectHandler<T> {
preconditionFailure("redirectURL doesn't contain a host")
}

if let redirectScheme = redirectURL.scheme {
request.scheme = redirectScheme
} else {
if redirectURL.scheme == nil {
preconditionFailure("redirectURL doesn't contain a scheme")
}

Expand Down
1 change: 1 addition & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientInternalTests+XCTest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import XCTest
extension HTTPClientInternalTests {
static var allTests: [(String, (HTTPClientInternalTests) -> () throws -> Void)] {
return [
("testRequestWithHost", testRequestWithHost),
("testHTTPPartsHandler", testHTTPPartsHandler),
("testHTTPPartsHandlerMultiBody", testHTTPPartsHandlerMultiBody),
("testProxyStreaming", testProxyStreaming),
Expand Down
Loading