Allow Kitura-NIO to only listen on one network address #205

TaborKelly · 2019-05-16T02:35:29Z

Description

@billabt was nice enough to add support for this at the BlueSocket layer here. However, I really want to use that functionality to fix Kitura/Kitura#1450.

I also have code ready to go for Kitura/Kitura-net#299 and Kitura.

Obviously, this is the Kitura-NIO version of the Kitura-Net change.

Motivation and Context

This is necessary to close Kitura/Kitura#1450. Should I open another issue for Kitura-NIO?

How Has This Been Tested?

Manual testing by myself. I also modified the test harness to use the new code path.

Checklist:

I have submitted a CLA form
If applicable, I have updated the documentation accordingly.
If applicable, I have added tests to cover my changes.

pushkarnk · 2019-05-17T06:11:16Z

Sources/KituraNet/HTTP/HTTPServer.swift

+        try listen(.tcp(port, node))
+    }
+
+    @available(*, deprecated, message: "use 'listen(on:node) throws' instead")


I'm not sure if we should deprecate listen(on:). We could make this a simple additive change by introducing a new listen(on:node) or listen(on:address). Or better have a default value for node/address

pushkarnk · 2019-05-17T06:14:57Z

Sources/KituraNet/HTTP/HTTPServer.swift

     - Parameter delegate: The delegate handler for HTTP connections.

     - Returns: A new instance of a `HTTPServer`.
    */
-    public static func listen(on port: Int, delegate: ServerDelegate?) throws -> ServerType {
+    public static func listen(on port: Int, node: String?, delegate: ServerDelegate?) throws -> ServerType {


Can we have the node/address parameter have a default value of nil? To make sure this is not a breaking change.

pushkarnk · 2019-05-17T06:19:15Z

Sources/KituraNet/Server/Server.swift

@@ -26,20 +26,40 @@ public protocol Server {
    /// Port number for listening for new connections.
    var port: Int? { get }

+    /// Has the same meaning as in `getaddrinfo()`.
+    var node: String? { get }


Can we call this address?

pushkarnk · 2019-05-17T06:19:56Z

Sources/KituraNet/Server/Server.swift

    /// A server state.
    var state: ServerState { get }

    /// Listen for connections on a socket.
    ///
    /// - Parameter on: port number for new connections (eg. 8080)
+    /// - Parameter node: has the same meaning as in `getaddrinfo()`
+    func listen(on port: Int, node: String?) throws


Repeat: node can have a default value of nil

pushkarnk · 2019-05-17T06:20:42Z

Sources/KituraNet/Server/Server.swift

+    /// Listen for connections on a socket.
+    ///
+    /// - Parameter on: port number for new connections (eg. 8080)
+    @available(*, deprecated, message: "use 'listen(on:node) throws' with instead")


repeat: can we avoid deprecation here?

pushkarnk · 2019-05-17T06:21:49Z

Sources/KituraNet/HTTP/HTTPServer.swift

@@ -58,6 +58,9 @@ public class HTTPServer: Server {
    /// The TCP port on which this server listens for new connections. If `nil`, this server does not listen on a TCP socket.
    public private(set) var port: Int?

+    /// Has the same meaning as in `getaddrinfo()`.
+    public private(set) var node: String?


Can we call this address please?

pushkarnk

Looks good. I request a couple of changes though:

As commented by @djones6 on the Kitura-net PR can we have node renamed as address?
All the new versions of listen() can be changed to include a new address parameter with the default value of nil. This is to make sure the change isn't breaking and there is no need to deprecate existing API method.

Update: Realised it isn't possible to have default parameter values with protocols, which can be worked around using extensions. At the least, we must make this a non breaking change i.e. we must add the new listen versions without deprecating the existing ones.

2. Remove extra code.

TaborKelly · 2019-05-18T21:30:35Z

I believe that my latest changes fix everything that was requested and bring this PR in line with the one for Kitura-Net. Let me know if I missed anything.

pushkarnk · 2019-05-23T07:04:01Z

Sources/KituraNet/HTTP/HTTPServer.swift

+     - Parameter address: The address of the network interface to listen on. Defaults to nil, which means this server
+                          will listen on all interfaces.
+     */
+    public func listen(on port: Int, address: String?) throws {


This must match the Kitura-net version:
public func listen(on port: Int, address: String? = nil) throws {

pushkarnk · 2019-05-23T07:05:04Z

Sources/KituraNet/HTTP/HTTPServer.swift

     - Parameter delegate: The delegate handler for HTTP connections.

     - Returns: A new instance of a `HTTPServer`.
    */
-    public static func listen(on port: Int, delegate: ServerDelegate?) throws -> ServerType {
+    public static func listen(on port: Int, address: String?, delegate: ServerDelegate?) throws -> ServerType {


Same feedback here. This must match Kitura-net:
public static func listen(on port: Int, address: String? = nil, delegate: ServerDelegate?) throws -> HTTPServer

pushkarnk

@TaborKelly Just a couple of more comments around the parity (with Kitura-net) of the two listen() methods.

djones6 · 2019-05-23T12:59:37Z

Sources/KituraNet/FastCGI/FastCGIServer.swift

-    public func listen(on port: Int) throws {
+    /// - Parameter address: The address of the network interface to listen on. Defaults to nil, which means this
+    ///             server will listen on all interfaces.
+    public func listen(on port: Int, address: String?) throws {


Although FastCGI is not implemented yet, the function signature should still be consistent with Kitura-net (as per @pushkarnk 's comments on HTTPServer) - need to add the default nil value.

djones6 · 2019-05-23T12:59:46Z

Sources/KituraNet/FastCGI/FastCGIServer.swift

    /// - Parameter delegate: the delegate handler for FastCGI/HTTP connections
    ///
    /// - Returns: a new `FastCGIServer` instance
-    public static func listen(on port: Int, delegate: ServerDelegate?) throws -> FastCGIServer {
+    public static func listen(on port: Int, address: String?, delegate: ServerDelegate?) throws -> FastCGIServer {


(same here)

Kitura-Net exactly.

TaborKelly · 2019-05-23T17:32:16Z

Good catch. I missed that since they conformed to the same Server protocol.

Pass through node to bootstrap.bind().

cfd7de7

TaborKelly mentioned this pull request May 16, 2019

Allow Kitura-Net to only listen on one network address Kitura/Kitura-net#299

Merged

3 tasks

DunnCoding requested a review from pushkarnk May 16, 2019 13:38

DunnCoding assigned pushkarnk May 16, 2019

pushkarnk reviewed May 17, 2019

View reviewed changes

pushkarnk suggested changes May 17, 2019

View reviewed changes

1. Change node to address.

2c8bebd

2. Remove extra code.

Better documentation comments.

279f321

pushkarnk reviewed May 23, 2019

View reviewed changes

pushkarnk suggested changes May 23, 2019

View reviewed changes

djones6 reviewed May 23, 2019

View reviewed changes

Make funtion signatures for FastCGIServer and HTTPServer match

9a46b2c

Kitura-Net exactly.

pushkarnk approved these changes May 24, 2019

View reviewed changes

pushkarnk merged commit 61b29ed into Kitura:master May 24, 2019

dannys42 unassigned pushkarnk Sep 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Kitura-NIO to only listen on one network address #205

Allow Kitura-NIO to only listen on one network address #205

TaborKelly commented May 16, 2019 •

edited

Loading

pushkarnk May 17, 2019 •

edited

Loading

pushkarnk May 17, 2019

pushkarnk May 17, 2019

pushkarnk May 17, 2019

pushkarnk May 17, 2019

pushkarnk May 17, 2019

pushkarnk left a comment •

edited

Loading

TaborKelly commented May 18, 2019

pushkarnk May 23, 2019

pushkarnk May 23, 2019

pushkarnk left a comment •

edited

Loading

djones6 May 23, 2019

djones6 May 23, 2019

TaborKelly commented May 23, 2019

Allow Kitura-NIO to only listen on one network address #205

Allow Kitura-NIO to only listen on one network address #205

Conversation

TaborKelly commented May 16, 2019 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Checklist:

pushkarnk May 17, 2019 • 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

pushkarnk left a comment • edited Loading

Choose a reason for hiding this comment

TaborKelly commented May 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pushkarnk 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

TaborKelly commented May 23, 2019

TaborKelly commented May 16, 2019 •

edited

Loading

pushkarnk May 17, 2019 •

edited

Loading

pushkarnk left a comment •

edited

Loading

pushkarnk left a comment •

edited

Loading