Basic support for unix domain sockets #187

pushkarnk · 2019-03-22T19:00:16Z

This is a replication of the unix domain socket support on Kitura-net. This pull request proposes an additive change to the HTTP.request() and ClientRequest.init(options:) APIs via the addition of a new optional argument named socketPath, which defaults to nil to ensure zero breakage of ClientRequest. That's about the client side support. For the sake of testing, we also have an internal listen(unixDomainSocket:) that provides server-side support for listening on unix domain sockets. One of the existing Kitura-NIO tests testLargePosts is modified to work with a unix socket.

codecov-io · 2019-03-24T11:52:53Z

Codecov Report

Merging #187 into master will increase coverage by 0.27%.
The diff coverage is 84.31%.

@@            Coverage Diff             @@
##           master     #187      +/-   ##
==========================================
+ Coverage   68.35%   68.63%   +0.27%     
==========================================
  Files          20       20              
  Lines        1324     1358      +34     
==========================================
+ Hits          905      932      +27     
- Misses        419      426       +7

Flag	Coverage Δ
#KituraNet	`68.63% <84.31%> (+0.27%)`	⬆️

Impacted Files	Coverage Δ
Sources/KituraNet/HTTP/HTTPRequestHandler.swift	`82.66% <ø> (-0.23%)`	⬇️
Sources/KituraNet/HTTP/HTTP.swift	`95.83% <100%> (ø)`	⬆️
Sources/KituraNet/HTTP/HTTPServer.swift	`57.33% <81.81%> (+3.01%)`	⬆️
Sources/KituraNet/ClientRequest.swift	`75.13% <87.5%> (+0.2%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad5e257...41062b6. Read the comment docs.

djones6

Although the initial purpose was to provide client-side support, I think it's worth making the server-side facility public also (since this implements it). We need to get the -net and -NIO implementations in sync with respect to this (and naming), then merge and tag both.

I'll work on the corresponding Kitura PR.

Sources/KituraNet/ClientRequest.swift

djones6 · 2019-03-26T14:24:29Z

Sources/KituraNet/HTTP/HTTP.swift

@@ -48,8 +48,8 @@ public class HTTP {
        return ClientRequest(url: url, callback: callback)
    }

-    public static func request(_ options: [ClientRequest.Options], callback: @escaping ClientRequest.Callback) -> ClientRequest {
-        return ClientRequest(options: options, callback: callback)
+    public static func request(_ options: [ClientRequest.Options], socketPath: String? = nil, callback: @escaping ClientRequest.Callback) -> ClientRequest {


This whole function seems to be missing API documentation... Kitura-net's doc is far from exemplary in this instance, but does attempt to describe the params.

A recently merged PR has copied all of the documentation comments from Kitura-net to Kitura-NIO :)

djones6 · 2019-03-26T14:29:39Z

Sources/KituraNet/ClientRequest.swift

@@ -207,8 +210,9 @@ public class ClientRequest {
    ///
    /// - Parameter options: An array of `Options' describing the request
    /// - Parameter callback: The closure of type `Callback` to be used for the callback.
-    init(options: [Options], callback: @escaping Callback) {
+    init(options: [Options], socketPath: String? = nil, callback: @escaping Callback) {


Let's call this parameter unixDomainSocketPath to be consistent.

djones6 · 2019-03-26T14:49:28Z

Sources/KituraNet/HTTP/HTTPServer.swift

@@ -34,6 +34,8 @@ public class HTTPServer: Server {
    /// Port number for listening for new connections.
    public private(set) var port: Int?

+    var unixDomainSocketPath: String?


In Kitura/Kitura-net#296, I went with

/// The Unix domain socket path on which this server listens for new connections. If `nil`, this server does not listen on a Unix socket. public private(set) var unixDomainSocketPath: String?

...as we provide a similar property for accessing the port. I also updated the API doc for port to include:

If nil, this server does not listen on a TCP socket.

djones6 · 2019-03-26T14:49:51Z

Sources/KituraNet/HTTP/HTTPServer.swift

@@ -160,11 +162,25 @@ public class HTTPServer: Server {
        return nil
    }

+    enum SocketType {


Should this be private?

djones6 · 2019-03-26T14:50:50Z

Sources/KituraNet/HTTP/HTTPServer.swift

+        case unixDomainSocket(String)
+    }
+
+    func listen(unixDomainSocketPath: String) throws {


This should be public to match public func listen(on port: Int). It could also do with documentation (could be a copy-paste of Kitura/Kitura-net#296)

djones6 · 2019-03-26T15:04:33Z

Tests/KituraNetTests/LargePayloadTests.swift

@@ -39,10 +39,25 @@ class LargePayloadTests: KituraNetTest {

    private let delegate = TestServerDelegate()

+    private func uniqueTemporaryFilePath() -> String {


This all looks fine, I used a slightly different implementation in the equivalent Kitura-net PR - take a look at my setUp() and tearDown() in Tests/KituraNetTests/UnixSocketTests.swift
- in particular, it would be nice to not leave a bunch of temporary files lying around after running the tests.

Also, is it possible for the test to assert that the client is connecting over a Unix socket? I did this in the test I referred to above, but I'm not sure if this is feasible with NIO.

…oLong

pushkarnk · 2019-03-28T18:12:12Z

Thanks for the review @djones6 I've addressed all of your comments.

Sources/KituraNet/HTTP/HTTPServer.swift

djones6

Changes look good to me now, however, I'd like to understand why the CI for Kitura is failing here: Kitura/Kitura#1434

I'm trying to run the whole Kitura test suite over Unix sockets. This works for Kitura-net, but the tests that send a redirect seem to be failing with NIO when using Unix sockets.

djones6 · 2019-03-29T16:35:21Z

@pushkarnk I pushed f04d4eb because it is otherwise not possible to start, stop and then restart the server. This also makes it consistent with BlueSocket, which removes the file at the socket path if it already exists.

djones6 · 2019-03-29T16:47:32Z

Sources/KituraNet/HTTP/HTTPServer.swift

-            serverChannel = try bootstrap.bind(host: "0.0.0.0", port: port).wait()
+            switch socket {
+            case SocketType.tcp(let port):
+                listenerDescription = "port \(port)"


I just realized this isn't quite right: this will result in port 0 when listening on an ephemeral port.

I've fixed this in 450882a

pushkarnk · 2019-03-31T09:57:34Z

@djones6 Redirection creates new ClientRequest objects. If an HTTP server listening on a unix domain socket returns a redirection request (302), the new ClientRequest must also connect to the same unix domain socket (if the value of the Location header is just another path i.e starts with /). If the value of the Location header is a full URL, we cannot make assumptions about it being hosted on the same unix domain socket.

This commit handles the case of the Location having just a different path 41062b6

With this the Kitura tests must pass.

pushkarnk changed the title ~~Basic support for unix domain sockets~~ [prototype]Basic support for unix domain sockets Mar 22, 2019

djones6 mentioned this pull request Mar 25, 2019

Add support for Unix sockets Kitura/Kitura-net#296

Merged

3 tasks

pushkarnk requested a review from djones6 March 26, 2019 05:57

pushkarnk mentioned this pull request Mar 26, 2019

Add Unix Domain Socket support in ClientRequest #185

Closed

djones6 suggested changes Mar 26, 2019

View reviewed changes

djones6 mentioned this pull request Mar 27, 2019

Support Unix domain sockets Kitura/Kitura#1434

Merged

2 tasks

Pushkar N Kulkarni added 3 commits March 28, 2019 17:28

Basic support for unix domain sockets

c7154c8

tests/macos: Restrict filename length to avoid unixDomainSocketPathTo…

998f1ef

…oLong

Address review comments

05370fe

pushkarnk force-pushed the unix-domain-sockets-support branch from 7602ff8 to 05370fe Compare March 28, 2019 18:08

Make listen(unixDomainSocket:) public

8cc86d0

Add a test that uses unix sockets

0518e18

djones6 reviewed Mar 29, 2019

View reviewed changes

Sources/KituraNet/HTTP/HTTPServer.swift Show resolved Hide resolved

Remove force-unwrap and avoid crash in log statements

f1c0489

djones6 self-requested a review March 29, 2019 16:07

djones6 reviewed Mar 29, 2019

View reviewed changes

Ensure Unix domain socket path does not exist

f04d4eb

djones6 reviewed Mar 29, 2019

View reviewed changes

Pushkar N Kulkarni added 2 commits March 30, 2019 08:05

Setting the listenerDescription right

450882a

uds: create the ClientRequest for redirected URL right

41062b6

pushkarnk changed the title ~~[prototype]Basic support for unix domain sockets~~ Basic support for unix domain sockets Mar 31, 2019

djones6 approved these changes Apr 1, 2019

View reviewed changes

Pushkar N Kulkarni and others added 2 commits April 1, 2019 19:15

tests: run client/server tests with unix sockets too

3dc7816

Add missing doSetUp() to initialize unix socket path

224d2bf

djones6 merged commit d403683 into master Apr 2, 2019

djones6 deleted the unix-domain-sockets-support branch April 2, 2019 16:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic support for unix domain sockets #187

Basic support for unix domain sockets #187

pushkarnk commented Mar 22, 2019

codecov-io commented Mar 24, 2019 •

edited

Loading

djones6 left a comment •

edited

Loading

djones6 Mar 26, 2019

pushkarnk Mar 28, 2019

djones6 Mar 26, 2019

djones6 Mar 26, 2019

djones6 Mar 26, 2019

djones6 Mar 26, 2019

djones6 Mar 26, 2019

pushkarnk commented Mar 28, 2019

djones6 left a comment

djones6 commented Mar 29, 2019

djones6 Mar 29, 2019

pushkarnk Mar 30, 2019 •

edited

Loading

pushkarnk commented Mar 31, 2019

		@@ -39,10 +39,25 @@ class LargePayloadTests: KituraNetTest {

		private let delegate = TestServerDelegate()

		private func uniqueTemporaryFilePath() -> String {

Basic support for unix domain sockets #187

Basic support for unix domain sockets #187

Conversation

pushkarnk commented Mar 22, 2019

codecov-io commented Mar 24, 2019 • edited Loading

Codecov Report

djones6 left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pushkarnk commented Mar 28, 2019

djones6 left a comment

Choose a reason for hiding this comment

djones6 commented Mar 29, 2019

Choose a reason for hiding this comment

pushkarnk Mar 30, 2019 • edited Loading

Choose a reason for hiding this comment

pushkarnk commented Mar 31, 2019

codecov-io commented Mar 24, 2019 •

edited

Loading

djones6 left a comment •

edited

Loading

pushkarnk Mar 30, 2019 •

edited

Loading