Misc fix test #248

dannys42 · 2021-03-14T21:50:44Z

No description provided.

CLAassistant · 2021-03-14T21:50:48Z

All committers have signed the CLA.

(this is typically normal)

…signment - SSL initalizer: NIOSSLClientHandler is now initialized with hostname - end() now assumes default ports based on scheme when not supplied in URL string. (Previously it always assumed 8080?!)

dannys42 · 2021-03-14T23:49:11Z

Woohoo! It's passing! Hopefully once merged I'll be able to get the Kitura tests to pass too.

mbarnach

I would feel better if we could avoid the force unwrapping, otherwise, good job!

mbarnach · 2021-03-15T08:16:33Z

Sources/KituraNet/ClientRequest.swift

@@ -535,11 +535,12 @@ public class ClientRequest {
     */
    public func end(close: Bool = false) {
        closeConnection = close
-
+        let percentEncodedURL = URL(string: self.percentEncodedURL)!


Are you sure about the force unwrapping? It looks dangerously un-recoverable.

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.

mbarnach · 2021-03-15T08:16:59Z

Sources/KituraNet/ClientRequest.swift

        var isHTTPS = false

        let group = MultiThreadedEventLoopGroup(numberOfThreads: 1)
-        if (URL(string: percentEncodedURL)?.scheme)! == "https" {
+        if (percentEncodedURL.scheme)! == "https" {


Same here, even if it was already forced before.

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.

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.

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?

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

mbarnach · 2021-03-15T08:21:16Z

.kitura-test.sh

@@ -10,11 +12,16 @@ if [ $SWIFT_TEST_STATUS -ne 0 ]; then
  return $SWIFT_TEST_STATUS
 fi

+# For now, short-circuit kitura tests until those are stabalized.
+return 0


I've create issue #249 to remember that...

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.

adam-rocska · 2021-03-16T09:00:36Z

.kitura-test.sh

@@ -10,11 +12,16 @@ if [ $SWIFT_TEST_STATUS -ne 0 ]; then
  return $SWIFT_TEST_STATUS
 fi

+# For now, short-circuit kitura tests until those are stabalized.
+return 0


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.

adam-rocska · 2021-03-16T09:04:03Z

.travis.yml

@@ -15,17 +15,23 @@ matrix:
      dist: xenial
      sudo: required
      services: docker
-      env: DOCKER_IMAGE=swift:5.0.3-xenial SWIFT_SNAPSHOT=5.0.3 DOCKER_PRIVILEGED=true SWIFT_TEST_ARGS="--parallel"
+      env: DOCKER_IMAGE=docker.kitura.net/kitura/swift-ci:5.0.3 SWIFT_SNAPSHOT=5.0.3 DOCKER_PRIVILEGED=true SWIFT_TEST_ARGS="--parallel"


adam-rocska · 2021-03-16T09:12:06Z

Sources/KituraNet/ClientRequest.swift

        var isHTTPS = false

        let group = MultiThreadedEventLoopGroup(numberOfThreads: 1)
-        if (URL(string: percentEncodedURL)?.scheme)! == "https" {
+        if (percentEncodedURL.scheme)! == "https" {


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.

adam-rocska · 2021-03-16T09:15:49Z

Sources/KituraNet/ClientRequest.swift

+                portNumber = 80
+            } else {
+                portNumber = 8080
+                Log.error("Unknown port for url: \(url)  Assuming 8080")


Well, technically it's not an error, since you have recovered in a transparent & seamless way.
I'd suggest implementing it as a warning instead of an error.

That's true... but at the same time I don't think having a default 8080 port makes any sense. I kept it there to avoid introducing more error paths and since the previous code had it. You think it should be a warning instead of an error? I'll add a comment to keep people from relying on this behavior.

adam-rocska · 2021-03-16T09:19:25Z

Sources/KituraNet/ClientRequest.swift

+                portNumber = 8080
+                Log.error("Unknown port for url: \(url)  Assuming 8080")
+            }
+        }
        if self.headers["Host"] == nil {
           let isNotDefaultPort = (portNumber != 443 && portNumber != 80) //Check whether port is not 443/80


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.

@dannys42 && @mbarnach what do you think?

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.

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?

The way I understand it is: instead of 80, you have something like .http and instead of 443, .https. For 8080, it would be .default (names are poorly chosen!). It could also be extended to use .localhost and .domain(let custom) for the hostnames. Feature wise, the same, but better to add semantic and group the value in a single place. As part of the port enum, the var isDefaultPort: Bool { self == .http || self == .https } could be introduced.

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?

…rred.

mbarnach · 2021-03-29T07:06:31Z

@dannys42 Shall we create the tickets to keep track of those and merge the PR?

dannys42 · 2021-04-02T01:13:29Z

Created an issue to keep track of the various topics discussed here: #250

…e with old version of xcode

sonarcloud · 2021-04-03T04:47:47Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

No Coverage information
0.0% Duplication

dannys42 · 2021-04-03T07:16:13Z

I'm not sure why, but with Xcode 10.3, swift-nio-ssl doesn't build properly. Looking at the source, it should be requiring macOS 10.15, but Travis-CI's Xcode 10.3 image is clearly running macOS 10.14. So I'm not sure why there was an issue. I'm just going to deprecate Xcode 10.3. We do see that Swift 5.0.1 still builds fine on Linux so I think this is probably fine.

dannys42 added 2 commits March 14, 2021 14:52

- Don't report error when removing a sockt file that does not exist

17304f8

(this is typically normal)

Fix bug in SSL initialization; Fix bug resulting in incorrect port as…

97c9b5b

…signment - SSL initalizer: NIOSSLClientHandler is now initialized with hostname - end() now assumes default ports based on scheme when not supplied in URL string. (Previously it always assumed 8080?!)

dannys42 force-pushed the MISC-fix_test branch from 20b55bd to 97c9b5b Compare March 14, 2021 21:53

dannys42 added 4 commits March 14, 2021 14:55

Travis: Update to include Linux swift 5.3.3 and macOS Xcode 12

8088489

Travis: Update to use private docker registry

edc772e

Disable kitura tests for now; update links to community github org

c8e8f54

Travis: Specify swift version for xcode 12.2

551b0f4

dannys42 requested review from adam-rocska and mbarnach March 14, 2021 23:48

mbarnach mentioned this pull request Mar 15, 2021

Ensure Kitura tests are run #249

Open

mbarnach requested changes Mar 15, 2021

View reviewed changes

adam-rocska reviewed Mar 16, 2021

View reviewed changes

dannys42 added 2 commits March 28, 2021 10:56

Add TODO to script comment

dd1ee8a

Add comment to error message when no port is specified or can be infe…

1f207b2

…rred.

mbarnach approved these changes Mar 31, 2021

View reviewed changes

dannys42 mentioned this pull request Apr 2, 2021

Possible internal improvements #250

Open

dannys42 added 2 commits April 2, 2021 21:08

travis: update xcode to 10.3, 11.3, and 11.6 to due to potential issu…

457fe8a

…e with old version of xcode

remove support for xcode 10.3

e3ef975

dannys42 merged commit d870257 into Kitura:master Apr 4, 2021

dannys42 deleted the MISC-fix_test branch April 4, 2021 05:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Misc fix test #248

Misc fix test #248

dannys42 commented Mar 14, 2021

CLAassistant commented Mar 14, 2021 •

edited

Loading

dannys42 commented Mar 14, 2021

mbarnach left a comment

mbarnach Mar 15, 2021

dannys42 Mar 28, 2021

mbarnach Mar 15, 2021

dannys42 Mar 15, 2021

adam-rocska Mar 16, 2021

dannys42 Mar 28, 2021

dannys42 Mar 28, 2021

mbarnach Mar 15, 2021

adam-rocska Mar 16, 2021

dannys42 Mar 30, 2021

adam-rocska Mar 16, 2021

adam-rocska Mar 16, 2021

dannys42 Mar 18, 2021

adam-rocska Mar 16, 2021

adam-rocska Mar 16, 2021

dannys42 Mar 28, 2021 •

edited

Loading

adam-rocska Mar 16, 2021

mbarnach Mar 17, 2021

dannys42 Mar 18, 2021

mbarnach Mar 19, 2021

dannys42 Mar 30, 2021

mbarnach commented Mar 29, 2021

dannys42 commented Apr 2, 2021

sonarcloud bot commented Apr 3, 2021

dannys42 commented Apr 3, 2021

Misc fix test #248

Misc fix test #248

Conversation

dannys42 commented Mar 14, 2021

CLAassistant commented Mar 14, 2021 • edited Loading

dannys42 commented Mar 14, 2021

mbarnach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannys42 Mar 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbarnach commented Mar 29, 2021

dannys42 commented Apr 2, 2021

sonarcloud bot commented Apr 3, 2021

dannys42 commented Apr 3, 2021

CLAassistant commented Mar 14, 2021 •

edited

Loading

dannys42 Mar 28, 2021 •

edited

Loading