Also I strongly recommend doing the UNIX socket stuff (new feature) in a later PR and leaving this just as a pure refactor.

Sure, I can split it up.

I'm not entirely sure about Context#socket but I'm not sure there is an alternative and I think the socket should be accessible from a handler when you can have different ones.

@ysbaddaden Unfortunately, I have pushed new commits while you were reviewing and your comments have been hidden but not all of them are resolved.

The #bind overloads are reduced to a minimum. I don't like having different method names for this because it's just not necessary when the arguments clearly convey the meaning. I don't see much sense in using different names for the same thing.

In your examples, there was also a mention of bind_ssl_listener - do you think we need tls based on individual sockets? I'd favour having the entire server run either with TLS or not.

I like the listen overload that forwards to bind. The difference between

server = HTTP::Server.new do |ctx|
  ctx.response.puts "Hello World!"
end

server.bind(80)
server.listen

and

server = HTTP::Server.new do |ctx|
  ctx.response.puts "Hello World!"
end

server.listen(80)

is actually quite large, as learning an ordering of methods - rather than simply a single method - is a fair bit more of a hurdle.

I do really want to keep the bind overloads to a minimum though.

It definitely has something to it. To me, the strongest point I see against it is the method signature. It would either delegate every single overload of #bind - which means a lot of overloads for #listen as well - or as I had it implemented, just catch the arguments and forward them to #bind. This is easier but #listen's method signature does no longer show what arguments it can accept.
But this might not be such a huge issue after all... hm. The docs can just list all possible overloads and/or reference #bind signatures.
In the end, it is a nice feature for about 90% of users who just need to listen on one socket.

Not sure I'm entirely convinced of it myself, but just as another possibility, we could also have bind, listen without arguments and a bind_and_listen that forwards its argument to bind for the common case

Hm, that would make the name for the common case very bulky...

I've removed the commit exposing socket on HTTP::Server::Context and will open a new issue for that.

Maybe we can find a less bulky but not too confusing name? start or serve would come to mind.

I doubt it would be helpful to introduce a different name. The main purpose is to listen - it just happens to also add a socket before that. We shouldn't have two method names for listening (which is also blocking).

What do you think about the return value of #bind_unused_port? It would make sense to return an IPAddress (ip + port) instead of only a port. That's especially true when called without a host argument, you shouldn't have to guess which IP is used. And the typical usecase will be printing the address (ip + port) like this:

address = server.bind_unused_port
puts "Listening on http://#{address}/"
server.listen

Even if the host is specified, it still makes sense to return the entire address (instead of having different return values depending on the presence of host argument).

And I guess it would be great to have access to the sockets ivar, or maybe only the local addresses of the sockets. If the server is listening on multiple sockets, the equivalent to the above sample could be:

# bind to a few sockets (maybe from config settings)

server.local_addresses.each do |address|
  puts "Listening on http://#{address}/"
end
server.listen

I've changed the return type of all bind and bind_unused_port method to Socket::IPAddress. That's what's typically interesting (to get the actual adress if port is zero) and there doesn't need to be direct access to the TCPServer. When this should really be necessary, it is possible to bind a custom TCPServer instance.

I'm not sure if bind(server : TCPServer) should maybe be renamed to add(server : TCPServer) or <<(server : TCPServer).

Starting individual TCP and SSL servers is half the point of having multiple listeners to me. Without it, your patch would be pointless in Prax for example (binds a single HTTP::Server to both HTTP and HTTPS).

Here is a confusing example:

server.bind("localhost")
server.bind("/tmp/app.sock")

A "but binding a TCP server requires to specify a port" is a bad answer, with a #bind(path : String) overload to bind an UNIXSocket, we'd introduce a confusing API, because the compiler won't complain, neither at compile time or runtime: it will bind an UNIXSocket when we expected a TCPSocket to some default port (e.g. 80).

I'd wan't to keep it a simple as possible. Keeping track of which sockets need to use TLS inside HTTP::Server seems unnecessarily tedious to me.

What about if we just use a special SSL server socket for this? It would essentially wrap another Socket::Server (TCPServer) and accepts returns a OpenSSL::SSL::Socket::Server wrapping a Socket instead of the TCPSocket directly.
This would make the use of TLS completely transparent from the perspective of the HTTP::Server and would be reusable as a drop-in replacement for a plain TCPServer (of course there should be a bind overload for setting this up which receives a SSL context).

@RX14 asked to introduce Unix sockets in a subsequent PR and I agree. This let's us focus on the general API first (obviously having upcoming Unix sockets in mind) and worry about Unix socket specifics separately.

Your confusing example doesn't work because you can't bind to a host without a port. It would be:

server.bind("localhost", 80)
server.bind("/tmp/app.sock")

The only overlap would be an overload accepting a String as generic URI like tcp://localhost:80, unix:///tmp/app.sock or ssl://0.0.0.0:442 vs. an overload that expects a String as path for a Unix socket.
So, yeah maybe this calls for differently named methods. The downside of this is a more complicated API (a few different method names instead of semantically matching overloads) and it wouldn't be as easy to forward calls from listen which equally helps to provide a simple API as per @RX14's comment.

I'd probably rather think about having a different overload for generic URI strings, only accepting URI type without a named argument or delegate this entirely to Socket::Address:

server.bind Socket::Address.parse "tcp://127.0.0.1:80"
server.bind Socket::Address.parse "unix:///var/run/socket"

You're right, #bind_unused_port doesn't add much over specifying port: 0 anymore. I still like it for it's explicit meaning instead of relying on a magic number.

Given that argless #listen is still there, wasn't the api of constructing the server with an initial/single host/port and then calling listen comfortable enough?

I fail to see the reason why to get rid of that constructor.

I think that easier/shorter interfaces for common use case are important. And add more fine grained ones to allow more rare scenarios, not at the expense of losing the easier ones.

@bcardiff It's comfortable, but such an additional signature for new would add four additional overloads. I'd like to avoid that. Using the listen short cut should be equally comfortable, more intuitive and easier to transform to bind calls:

server = HTTP::Server.new(host, port) { }
server.listen
# vs.
server = HTTP::Server.new { }
server.listen(host, port)

I get the api is not that different. But when creating a http server is usually with a host and port bind.
And the usually here means that only one overload is needed.

Having a initialize(host : String, port : Int32, &handler) even without the default ip will allow simpler introduction to the http server IMO.

I don't care that much the api is breaking, but the fact that is a nice simpler api for lot's of simple use case where a framework is not used.

You'd need to have these four additional overloads, even if you just want to support host : String, port : Int32. otherwise the API would be completely arbitrary.

• self.new(host : String, port : Int32, &handler : HTTP::Handler::Proc)
• self.new(host : String, port : Int32, handlers : Array(HTTP::Handler), &handler : HTTP::Handler::Proc)
• self.new(host : String, port : Int32, handlers : Array(HTTP::Handler))
• initialize(host : String, port : Int32, handler : HTTP::Handler | HTTP::Handler::Proc)

I don't really see how specifying the host and port in the constructor is a simpler introduction to the http server than in the #listen method (see my previous post). I rather think it's the other way around.

I was thinking just in the first overload. I haven't seen the other used that much in presentations and smalls projects. But, ok. Let's keep the proposal (and update website homepage sample on release 😉 )

@RX14 The squashed commit message of 01a5126 just joined all individual commit messages together, including the fixups. It's not a big deal but it would be preferable if at least the fixups were removed as they don't add anything to the message. (for future merges)