-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Refactor HTTP::Server
to bind to multiple addresses
#5776
Refactor HTTP::Server
to bind to multiple addresses
#5776
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.
I'd call them sockets not interfaces in the http server. You call them sockets in the http context.
src/http/server.cr
Outdated
|
||
def self.new(port, &handler : Context ->) | ||
new("127.0.0.1", port, &handler) | ||
# DEPRECATED: Create an instance and call `#bind` instead. |
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.
We don't deprecate before 1.0, we just remove. Unless it's super duper common and used in many places in one project (think MemoryIO
), and even then we make the overloads print notices using macros otherwise nobody will notice.
src/http/server.cr
Outdated
def bind(address : String, reuse_port : Bool = false) : Socket::Server | ||
if address.starts_with?("unix:") | ||
{% if flag?(:unix) %} | ||
bind(Socket::UNIXAddress.new(address[5..-1])).as(Socket::Server) |
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'm not sure we want to continue spattering ifdefs around the codebase. If sockets aren't supported, we'll raise in UNIXAddress.new
instead of everywhere it's used. Indent is wrong.
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.
Yeah, it's probably better this way. The whole Socket API looks like it needs a major refactoring anyway.
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.
Some duplication, here. Having different #bind
overloads is already redundant, but introducing yet another one with a URI-like string, required for binding UNIX sockets, that feels too much and weirdly designed.
It should either be the only solution, or there should be explicitly named methods (ala puma), which IMHO feels better (an application may require a URI, parse it, and call the correct methods):
#bind_tcp_listener(host, port)
#bind_ssl_listener(host, port, ctx)
#bind_unix_listener(host, port)
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 UNIX socket part is removed from this PR. Regarding method naming, I'd suggest to continue the discussion in the main thread.
src/http/server.cr
Outdated
# Creates a `Socket::Server` listenting to *address* and adds it as an interface. | ||
# | ||
# The *address* can either be `host:port` or a Unix socket prefixed by `unix:`. | ||
# This obviously works only on systems supporting Unix sockets. |
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.
"obviously" is unneccesary.
src/http/server.cr
Outdated
end | ||
end | ||
|
||
# Creates the underlying `TCPServer` if the doesn't already exist. | ||
{% if flag?(:unix) %} |
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.
Don't bother.
src/http/server.cr
Outdated
until @wants_close | ||
spawn handle_client(server.accept?) | ||
@interfaces.each do |interface| | ||
spawn handle_client(interface.accept?) |
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.
This is completely wrong. It will round-robin each interface accepting one connection from each interface in order, instead of which interface is ready to accept. You need to spawn a fiber for each interface.
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.
Darn. I'll put my shame hat on and hide in the basement... :(
The worst of it is that I had a relevant spec for this which somehow got lost during rebasing.
src/http/server/context.cr
Outdated
@@ -7,8 +7,11 @@ class HTTP::Server | |||
# The `HTTP::Server::Response` to configure and write to. | |||
getter response : Response | |||
|
|||
# An optional `Socket` which holds the client connection of this context. | |||
getter socket : Socket? |
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'm not sure about this.
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 here. Hijacking the IO must be achieved within a request handler or response output (see WebSocket handler).
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 websocket handler really hijacks the IO using upgrade
. The problem I'm trying to solve is not about taking over the IO handling (including responsibility for calling #close
). It's rather to get information about the connection, such as local_address
. Don't know about other things... remote_address
could be useful.
So maybe we could just expose those in Response
?
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'd prefer to expose specific metadata regarding the socket, instead of an actual socket you can perform IO on. That seems to be more like work that should be done in another PR though.
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 |
a20b633
to
91b7a9a
Compare
HTTP::Server
to bind to multiple addresses and support Unix socketsHTTP::Server
to bind to multiple addresses
91b7a9a
to
2612dcd
Compare
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.
Interesting, and going the right way, but:
- the constructor methods should be dropped;
- there are far too many
#bind
overloads inducing lots of duplication, a handful of them would be far enough; I'd prefer explicitly named methods, too (see comment below).
src/http/server.cr
Outdated
# You may set *reuse_port* to true to enable the `SO_REUSEPORT` socket option, | ||
# which allows multiple processes to bind to the same port. | ||
def bind(port : Int32, reuse_port : Bool = false) : TCPServer | ||
bind "127.0.0.1", port, reuse_port |
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 gonna say that binding to 127.0.0.1
makes the #bind(port)
overload somewhat useless (invisible on network, doesn't bind ipv6 interfaces) but that's how it's currently done...
That's still weird, thought, since TCPServer.new(port)
will bind on ::
(all ipv4 & ipv6 interfaces) for example.
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.
Yes, this should probably be improved. Although I'm not entirely sure if ::
would be a sane default.
I'd suggest to keep the current behavior for now and open an issue about this to fix it once this is merged. WDYT?
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 prefer to keep everything local-addressed by default, and force people to choose possibly insecure defaults (0.0.0.0, ::) by conscious decision.
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.
Still, we should make sure to take the IPv6 loopback ::1
into account as well. It would probably nice to have a constant or class method in Socket::IPAddress
instead of using magic value here.
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.
Maybe we could have a method to listen on 0.0.0.0
and ::
, like #bind_all(port : Int32)
src/http/server.cr
Outdated
def bind(address : String, reuse_port : Bool = false) : Socket::Server | ||
if address.starts_with?("unix:") | ||
{% if flag?(:unix) %} | ||
bind(Socket::UNIXAddress.new(address[5..-1])).as(Socket::Server) |
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.
Some duplication, here. Having different #bind
overloads is already redundant, but introducing yet another one with a URI-like string, required for binding UNIX sockets, that feels too much and weirdly designed.
It should either be the only solution, or there should be explicitly named methods (ala puma), which IMHO feels better (an application may require a URI, parse it, and call the correct methods):
#bind_tcp_listener(host, port)
#bind_ssl_listener(host, port, ctx)
#bind_unix_listener(host, port)
src/http/server.cr
Outdated
# server = HTTP::Server.new { } | ||
# server.bind Socket::UNIXAddress.new("/tmp/my-socket") | ||
# ``` | ||
def bind(address : Socket::UNIXAddress) : UNIXServer |
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.
Are the XAddress overloads really useful?
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 guess we don't necessarily need them, it's just that the Socket API is not really nice in that regard, you currently can't directly create a UNIXSocket from a UNIXAddress. But this needs to be fixed there (see #5778) then we can probably do without the overloads. Or have only a general bind(address : Socket::Address)
which delegates creating the appropriate server to Socket::Server.new(address : Socket::Address)
. That would probably be the best solution.
src/http/server.cr
Outdated
end | ||
|
||
# Adds a `Socket::Server` *interface* to this server. | ||
def bind(interface) |
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.
There should be a #bind(interface : Socket::Server)
type constraint.
src/http/server.cr
Outdated
|
||
# Binds to the specified arguments and starts the server. | ||
# | ||
# See `#bind` and `#listen` for details |
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.
Is it really helpful? We're introducing and require explicit #bind
but in the same time we mix its logic (bind an interface) with #listen
(for incoming connections).
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.
It's somewhat nice but not really necessary, I think. I guess we don't need it and it convolutes the API.
I will remove it. If desired, it can be added later.
src/http/server.cr
Outdated
@server = nil | ||
|
||
@interfaces.each do |interface| | ||
interface.close |
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.
Closing an interface can raise, which will prevent further interfaces from being closed.
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.
Will wrap it in rescue. We can probably ignore any exceptions, right?
src/http/server/context.cr
Outdated
@@ -7,8 +7,11 @@ class HTTP::Server | |||
# The `HTTP::Server::Response` to configure and write to. | |||
getter response : Response | |||
|
|||
# An optional `Socket` which holds the client connection of this context. | |||
getter socket : Socket? |
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 here. Hijacking the IO must be achieved within a request handler or response output (see WebSocket handler).
@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 In your examples, there was also a mention of |
f88addb
to
83f40d0
Compare
I like the 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 |
Not sure I'm entirely convinced of it myself, but just as another possibility, we could also have |
Hm, that would make the name for the common case very bulky... |
83f40d0
to
3d3b92c
Compare
I've removed the commit exposing |
Maybe we can find a less bulky but not too confusing name? |
I doubt it would be helpful to introduce a different name. The main purpose is to |
What do you think about the return value of 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 And I guess it would be great to have access to the # bind to a few sockets (maybe from config settings)
server.local_addresses.each do |address|
puts "Listening on http://#{address}/"
end
server.listen |
c6306a4
to
013a078
Compare
I've changed the return type of all |
I'm not sure if |
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). |
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.
Why drop the binding to UNIX sockets? Since we're refactoring this, I'd introduce new bindings, for example selectively binding SSL and binding UNIX sockets, or at least have a defined design for namings.
If we only bind TCPSocket then #bind
is enough, but what if we want to bind a UNIXSocket? or selectively have a SSL context for different servers? I disagree that #bind
and overloads convey enough meaning over proper naming (#bind_unix(path)
, #bind_tcp(host, port)
and #bind_ssl(host, port, ctx)
) with maybe a URI-like generic (#bind("tcp://host:port")
).
Last pedantic: #bind_unused_port
doesn't bring much benefit over #bind_tcp(port: 0)
.
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 |
013a078
to
9c0ea8e
Compare
I'd wan't to keep it a simple as possible. Keeping track of which sockets need to use TLS inside What about if we just use a special SSL server socket for this? It would essentially wrap another @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 I'd probably rather think about having a different overload for generic URI strings, only accepting server.bind Socket::Address.parse "tcp://127.0.0.1:80"
server.bind Socket::Address.parse "unix:///var/run/socket" You're right, |
src/http/server.cr
Outdated
|
||
# Starts the server. Blocks until the server is closed. | ||
def listen | ||
raise "Can't start server with not sockets to listen to" if @sockets.empty? |
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.
Maybe we should include something like "Use HTTP::Server#bind" in the error so we can guide the user a bit. WDYT?
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.
Sure.
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.
Another issue, which should probably also raise, is when bind
is called after listen
:
server.bind socket
spawn { server.listen }
server.bind socket2
For this we'd need a ivar to track if the server is listening. It could probably be implemented to retroactively start listening on the additional socket as well, but that's more complicated and probably not that useful without having service objects.
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.
Yeap, I would say to add an ivar to track that too.
Given that argless 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 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. Having a 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. |
96544b6
to
b3bd139
Compare
You'd need to have these four additional overloads, even if you just want to support
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 |
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 😉 ) |
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.
Also I think that the standard exception style is without a full-stop.
src/http/server.cr
Outdated
@@ -159,6 +162,9 @@ class HTTP::Server | |||
|
|||
# Adds a `Socket::Server` *socket* to this server. | |||
def bind(socket : Socket::Server) : Nil | |||
raise "Can't add socket to running server." if listening? | |||
raise "Can't add socket to closed server." if @wants_close |
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.
Just disallow #close
unless listening?
. I'd also just rename this variable to @closed
because the state persists after it's fully shut down - not just when it's in some "closing down" state that "wants close" implies.
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.
You need to be able to call #close
even if the server is not listening because sockets may already be bound and need to be released.
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.
having getter? closed
would be nice too.
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.
Already in there ;)
b3bd139
to
8e17445
Compare
) * Refactor HTTP::Server to use multiple interfaces * Remove HTTP::Server#port and add HTTP::Server#bind_unused_port The return type is now an Socket::IPAddress instead of only the port number. * Rename `HTTP::Server#bind` to `#bind_tcp`, add overloads to `#listen` * Add HTTP::Server#addresses * fixup! Refactor HTTP::Server to use multiple interfaces * fixup! Refactor HTTP::Server to use multiple interfaces * Add HTTP::Server.listening? and raise errors when running or closed
Until now, a
HTTP::Server
instance binds to exactly one IP address (host + port). This address was set in the constructor alongside several options for passing one or more handlers.This resulted in quite a few constructors already, and adding support for more options (such as using a Unix socket) would multiply the numbers. Additionally, it should be possible to have the server listen on multiple addresses, as many other HTTP server implementations.
This PR changes the API of
HTTP::Server
:#bind
is used to add one ore multiple addresses the server should listen on. It accepts the same arguments previously used in the constructor (port
,host, port
- each with optionalreuse_port
).Socket::IPAddress
andSocker::Server
.You can bind to a Unix socket using(Add HTTP::Server#bind_unix #5959 )#bind(Socket::UNIXAddress)
or by supplying aUNIXServer
There is also a single-argument overload that accepts a string with either a host + port or a Unix socket (prefixed by(will be a subsequent PR => Add HTTP::Server#bind(URI) #6500)unix:
) and creates the appropriate server accordingly. See #2735 (issue-comment) for the reasoning behind this.#bind_unused_port
is a shortcut forbind(0)
and returns the port. This is particularly useful for the use case of binding to an unused port and printing the port (could maybe change the return type toSocket::IPAddress
to include the address as well).#port
was removed because there is no single port anymore that this method could return.#listen
starts accepting incoming requests, as before. However, there is also an catch-all overload which forwards all arguments to#bind
and starts listening. This is a shortcut for the following:With a server listening on multiple interfaces, I figured it would be useful to be able to determine from which connection a client request originates, so I added a nilable (for stubbing) instance variable
socket
toHTTP::Server::Context
.This also includes a minor fix to copy the path of a UNIXServer to each socket. Otherwise you couldn't really tell them apart if (only by the file descriptor).(#5869)Fixes one half of #2735(#5959)