-
Notifications
You must be signed in to change notification settings - Fork 79
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.
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
Allow Kitura-Net to only listen on one network address #299
Conversation
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.
Thank you for the PRs! Some comments on the duplication of functions which I think can be avoided.
Also, I'm not sure that node
would be my choice of parameter name - could we call it address
instead?
- Parameter on: port number for new connections | ||
*/ | ||
@available(*, deprecated, message: "use 'listen(on:node) throws' instead") | ||
public func listen(on port: Int) throws { |
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.
The addition of the optional function parameter is non-breaking, so I don't think it's necessary to duplicate the functions here. In the (port:node:delegate:) case below, you'd need to default node to nil.
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 was trying not to break the public Server
protocol where default initializers are not allowed.
Are you saying that I can have them both (listen(on)
and listen(on:node)
) defined in the Server protocol but only listen(on:node)
with a default initializer in the actual implementation? If so, that would surely be preferable.
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.
If I remove listen(on)
from HTTPServer
but leave it in the Server
protocol definition (where I can't have a default initializer) swift isn't happy:
$ swift build
/Users/tabor.kelly/work/taborkelly/Kitura-Net/Sources/KituraNet/HTTP/HTTPServer.swift:41:14: error: type 'HTTPServer' does not conform to protocol 'Server'
public class HTTPServer: Server {
^
/Users/tabor.kelly/work/taborkelly/Kitura-Net/Sources/KituraNet/Server/Server.swift:45:10: note: protocol requires function 'listen(on:)' with type '(Int) throws -> ()'; do you want to add a stub?
func listen(on port: Int) throws
Please note, this is with the default initializer in HTTPServer.swift:
public func listen(on port: Int, node: String? = nil) throws {
self.port = port
self.node = node
try listen(.inet(port, node))
}
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.
Hmm. A known work around to this is writing a protocol extension with a method that has the default valued parameter.
extension Server {
func listen(on port: Int, node: String? = nil) {
listen(on: port, node: node)
}
}
} | ||
|
||
@available(*, deprecated, message: "use 'listen(on:node) throws' instead") | ||
public func listen(on port: Int) throws { |
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.
Same comment as for FastCGIServer - shouldn't need two functions as long as node
is given a default of nil
As for naming, I am happy to change |
@TaborKelly I tried out @pushkarnk's suggestion with the protocol extension here, and it seems to work: Here's a quick hack up of the corresponding Kitura change to surface the new option: |
2. Remove extra code.
Thanks of the help. djones, I used your code almost verbatim. I only fixed up some comments. I think the changed (stripped) whitespace was in your version, and I just copied it. I'm fine with it if you are, but I am also willing to re-add trailing whitespace depending on what you folks want. |
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.
The changes look good, however could we change the documentation comment for address
to be more descriptive (I've given some suggestions). There are several instances of the same comment that would need updating, and similarly for the NIO PR.
```` | ||
|
||
- Parameter port: Port number for new connections, e.g. 8080 | ||
- Parameter address: has the same meaning as node in `getaddrinfo()` |
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.
Could we document this parameter as:
The address of a network interface to listen on, for example
"localhost"
. The default isnil
, which listens for connections on all interfaces.
@@ -55,12 +55,15 @@ 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 node in `getaddrinfo()`. |
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.
How about
The address of the network interface to listen on. Defaults to
nil
, which means this server will listen on all interfaces.
I think that I changed all the documentation comments to match your suggested wording. If we wanted to we could change the word |
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-NIO#205 and Kitura.
Motivation and Context
This is necessary to close Kitura/Kitura#1450. Should I open another issue for Kitura-Net?
How Has This Been Tested?
Manual testing by myself. I also modified the test harness to use the new code path.
Checklist: