Move HTTPServer options from Start into init

[gtaban]

Remove arguments from HTTPServer start function and put it in an option class and pass in init.

[helje5]

My opinions attached ;-)

[helje5]

I would keep the handler out of the options. The options should describe the server environment. The handler does the handling.

[helje5]

I think this should be init(handler: .., options:). The with style is for method names, not for init (I think, not sure).

[gtaban]

For the names, I followed the patterns of dataTask.
dataTask(with:)
https://developer.apple.com/documentation/foundation/urlsession/1411554-datatask
dataTask(with:completionHandler:)
https://developer.apple.com/documentation/foundation/urlsession/1410330-datatask

[seabaylea]

I agree with moving to options, particularly as we might have a large set with TLS added.

I'm wondering about the move of the handler into init() from start() though. A side effect of this is that it makes it look weird (but not wrong) if you declare an anonymous/inline closure for the handler, e.g.

        let server = HTTPServer(port: 0) {request, response in
            response.writeHeader(status: .ok)
            response.writeBody("Hello, World!")
            response.done()
            return .discardBody 
        }

Of course, its a reasonable argument that we intentionally want to discourage the use of anonymous closures...

[helje5]

I would say your example actually looks quite nice and matches what you'd do in Foundation.

Part of my reasoning of moving the handler and such to init is that they can be let constants and no synchronisation needs to happen when accessing them.

(P.S./: don't do: port: 0, that should raise an error. Do port: nil for kernel assigned, or a real port)

[gtaban]

Thanks @helje5 , @seabaylea, for your comments. I'll move handle out of options.
I'll create another PR with for port = nil for kernel assigned ports, since changes would be all over the place.

[seabaylea]

@gtaban remember to keep the handler as the last option, so that its possible to define an anonymous closure (if you want to).

For 0 vs nil, the trade off is that allowing nil means using an Optional and having to do a whole bunch of nil checks. Rather than supporting 0 for random port (which is wrong as 0 is actually a reserved port), I think the question is whether there's a real need for kernel assigned port.

The kernel assign port is probably useful for testing, but is it required there (or for any other use cases)?

[helje5]

Kernel assigned ports are very useful, I use them all the time for plenty of scenarios (backend servers, on-device per-app servers, etc.).
It is more like the other way around - assigned ports are only really useful for front facing servers. And whether Swift will replace Apache/NGINX/your-load-balance-of-choice soon is yet to be seen ;-) I mean,. we don't even have sendfile support (!), so you need something to deliver static resources efficiently anyways.

[helje5]

Oh, and having said that. port is really insufficient in the first place. We need to allow an (or many!) arbitrary sockaddr_in (either via sockaddr_in or a wrapper). If only to allow ipv6 and such. af_local is very useful too.

I have a small set of extensions for the Posix that make them really easy to use as-is (wildcard binding, address binding, all that). Feel free to integrate them:

https://github.com/NozeIO/Noze.io/blob/master/Sources/net/SocketAddress.swift#L27

[seabaylea]

Both Node.js and Golang use the convention of 0 for kernel assigned port, so I guess its a common convention...

Given that I assume Noze uses nil, do you think the need to use Optionals and nil check is a concern?

[helje5]

Hm, I guess I don't care too much. An optional port seems like the correct choice for Swift to me, but well. (in the end, sockaddr_in also uses 0 for the wildcard port ...)

I think it is more important to not use a plain port, but a sockaddr. Because the same way like you want to do wildcard ports, you want to do wildcard addresses, or not, or different addresses. (not wanting to do non-wildcard addresses seems even more important than wildcard ports to me - aka bind to specific server interfaces)

[seabaylea]

Agreed - we definitely need support for full addresses to be passed in via Options.

[helje5]

An open question is, do we want to support multiple binds? I tend to say yes. This would modify the structure of the server a little.
(the other option is to use multiple servers if you want to listen on multiple addresses, but that would be kinda wasteful).

[anayini]

Can the type of port be UInt16?

[helje5]

It should not be a port in the first place but either a sockaddr or [sockaddr], port should only be a convenience initialiser.

In consequence your question would be answered (depends on the address type, protocol and sometimes the OS).

[anayini]

Can this be a struct?

[helje5]

Good catch, it should be a struct if the class is public anyways. It should be open instead. (Same maybe for HTTPServer.)

Also: the comment is wrong /// A subclass for HTTP Options. This is not a subclass but a root class.

Struct vs Class: Depends a little on whether we want to allow subclassing. The idea to use a class comes from my 2017-10-30 email:

class HTTPServer {
 class Options { // a class because subclasses may want to add

It could be a struct, but this would require wrapping if you add fields to it (very common in JS style options arrays). E.g. a framework on top, which wraps HTTPServer, could still use the same configuration, but add a few things (say a logger, or some template path).

Personally I don't care to much in this case. Either would be fine for me. But if it is a class, it needs to be open, that is the whole point.

[anayini]

👍

[gtaban]

I like the idea of open class for options (for example, it can be used by PoCSocketSimpleServer) but then should Options be defined within HTTPServer? (we do get namespacing, but will it be cleaner if Options is defined outside of HTTPServer?

Also what use is open Options but only public HTTPServer?

Good pick up on the comment. Thanks!

[helje5]

Also what use is open Options but only public HTTPServer.

public class MyExpressServer {
  open class Options : HTTPServer.Options {
    let templatePath : String
  }
  let http : HTTPServer
}

But maybe it should be just a struct. Seems cleaner for such an isolated project.

P.S.: We could have a general discussion of whether we want the classes open or closed. I'm generally in favour of open, because you never know, but Swift style may be different.

[gtaban]

Your example is exactly what I wanted to use for PoCSockerSimpleServer that I mentioned.
And what sold me on making options open. :-P

[gtaban]

I have opened Issues #88 for Port=0-->nil issue and #89 for use of sockaddr instead of port.
Please comment there on those issues.

[gtaban]

Opened #91 to track the issue of multiple binds per server vs multiple servers for each port

[gtaban]

Updated based on comments. @helje5 can you verify?

[ddunbar]

I agree with @helje5 that using an Optional (sockaddr wrapper) for kernel-assigned port feels like the right choice for Swift.