Misc fix test #248
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -535,11 +535,12 @@ public class ClientRequest { | |
| */ | ||
| public func end(close: Bool = false) { | ||
| closeConnection = close | ||
| let percentEncodedURL = URL(string: self.percentEncodedURL)! | ||
|
There was a problem hiding this comment. Are you sure about the force unwrapping? It looks dangerously un-recoverable. There was a problem hiding this comment. I'm also a bit leery about it, but I was opting for minimal changes. I think it will take quite a bit more restructuring to better handle these situations. |
||
|
|
||
| var isHTTPS = false | ||
|
|
||
| let group = MultiThreadedEventLoopGroup(numberOfThreads: 1) | ||
| if (percentEncodedURL.scheme)! == "https" { | ||
|
There was a problem hiding this comment. Same here, even if it was already forced before. There was a problem hiding this comment. Yeah, I don't know how you'd recover from a missing scheme? But I'll take another look... maybe there's a better way. There was a problem hiding this comment. If we'd have a huge concrete wall of tests, I'd be happy with it, but in its current state my lack of understanding and the code I see scares me too. But I don't have any better approaches right now either. Personally I think force unwrapping is fine if there are multiple good quality and self validating unit tests which reason about the implementation and is convincing of its safety. Otherwise I'd take the route of being super-defensive; and log warnings & errors whenever the sad path kicks in. There was a problem hiding this comment. My goal right now is just to get tests consistently passing. So if you can agree that I at least haven't introduced any new problems, can we save the larger restructuring until later? There was a problem hiding this comment. Curiously the existing code does not use URL() as it's base way of storing URLs. I think one reason for some of the complexity in the code is the way Kitura handles http/https vs Unix domain sockets. If we're keeping this functionality (which I think is nice to have), it might make some of the code easier to follow one of these (non-standard) conventions for naming Unix domain sockets: whatwg/url#577 |
||
| isHTTPS = true | ||
| self.sslConfig = TLSConfiguration.forClient(certificateVerification: .none) | ||
| } | ||
|
|
@@ -550,8 +551,20 @@ public class ClientRequest { | |
| initializeClientBootstrap(eventLoopGroup: group) | ||
| } | ||
|
|
||
| let hostName = percentEncodedURL.host! //TODO: what could be the failure path here | ||
| let portNumber: Int | ||
| if let port = percentEncodedURL.port { | ||
| portNumber = port | ||
| } else { | ||
| if percentEncodedURL.scheme == "https" { | ||
| portNumber = 443 | ||
| } else if percentEncodedURL.scheme == "http" { | ||
| portNumber = 80 | ||
| } else { | ||
| portNumber = 8080 | ||
| Log.error("Unknown port for url: \(url) Assuming 8080. This may change in the future.") | ||
| } | ||
| } | ||
| if self.headers["Host"] == nil { | ||
| let isNotDefaultPort = (portNumber != 443 && portNumber != 80) //Check whether port is not 443/80 | ||
|
There was a problem hiding this comment. This whole play of port literals gave me an idea: We could introduce a small harmless enum in one of the Kitura base modules which would encapsulate the typical ports and give names to them? The bigger benefit is that it could open the door for future additional features to port handling without change risks. There was a problem hiding this comment. I like the idea: it makes it more obvious (like which port is the default for Kitura), but also keep tracks of standard and custom ports in a robust fashion. There was a problem hiding this comment. Hmm.. you mean an enum equivalent to the /etc/services file? Might be interesting, but not sure it belongs in Kitura since Kitura really only deals with web traffic? There was a problem hiding this comment. The way I understand it is: instead of 80, you have something like There was a problem hiding this comment. Another idea, if we're using URL() as a base type, then adding an extension for "isStandardPort" where we can check that if a port was given that it matches the standard for the given scheme. It might also be interesting to do a run-time check of /etc/services rather than having a hard-coded database. Anyway I think this is a future project. Okay to punt on this for now? |
||
| self.headers["Host"] = hostName + (isNotDefaultPort ? (":" + String(portNumber)) : "") | ||
|
|
@@ -639,7 +652,7 @@ public class ClientRequest { | |
| bootstrap = ClientBootstrap(group: eventLoopGroup) | ||
| .channelOption(ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEADDR), value: 1) | ||
| .channelInitializer { channel in | ||
| channel.pipeline.addHandler(try! NIOSSLClientHandler(context: self.sslContext!, serverHostname: self.hostName)).flatMap { | ||
| channel.pipeline.addHTTPClientHandlers().flatMap { | ||
| channel.pipeline.addHandler(HTTPClientHandler(request: self)) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've create issue #249 to remember that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest to add a
// TODO:statement as well. Most IDEs collect and aggregate them. I for example use CLion, and this way I won't lose track of unkept promises.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.