Refactor `HTTP::Server` to bind to multiple addresses by straight-shoota · Pull Request #5776 · crystal-lang/crystal

straight-shoota · 2018-03-05T20:54:56Z

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:

The constructor only accepts handlers (in multiple variations, as before).
#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 optional reuse_port).
Additional overloads exist for Socket::IPAddress and Socker::Server.
~~You can bind to a Unix socket using #bind(Socket::UNIXAddress) or by supplying a UNIXServer~~ (Add HTTP::Server#bind_unix #5959 )
There is also a single-argument overload that accepts a string with either a host + port or a Unix socket (prefixed by unix:) and creates the appropriate server accordingly. See #2735 (issue-comment) for the reasoning behind this. (will be a subsequent PR => Add HTTP::Server#bind(URI) #6500)
#bind_unused_port is a shortcut for bind(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 to Socket::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:

server.bind 80
server.listen
# same as
server.listen 80

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 to HTTP::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)

RX14

I'd call them sockets not interfaces in the http server. You call them sockets in the http context.

RX14 · 2018-03-06T00:27:29Z

src/http/server.cr


-  def self.new(port, &handler : Context ->)
-    new("127.0.0.1", port, &handler)
+  # DEPRECATED: Create an instance and call `#bind` instead.


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.

RX14 · 2018-03-06T00:30:23Z

src/http/server.cr

+  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)


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.

Yeah, it's probably better this way. The whole Socket API looks like it needs a major refactoring anyway.

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)

The UNIX socket part is removed from this PR. Regarding method naming, I'd suggest to continue the discussion in the main thread.

RX14 · 2018-03-06T00:31:00Z

src/http/server.cr

+  # 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.


"obviously" is unneccesary.

RX14 · 2018-03-06T00:32:04Z

src/http/server.cr

  end

-  # Creates the underlying `TCPServer` if the doesn't already exist.
+  {% if flag?(:unix) %}


Don't bother.

RX14 · 2018-03-06T00:34:01Z

src/http/server.cr

    until @wants_close
-      spawn handle_client(server.accept?)
+      @interfaces.each do |interface|
+        spawn handle_client(interface.accept?)


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.

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.

RX14 · 2018-03-06T00:34:54Z

src/http/server/context.cr

    getter response : Response

+    # An optional `Socket` which holds the client connection of this context.
+    getter socket : Socket?


I'm not sure about this.

Same here. Hijacking the IO must be achieved within a request handler or response output (see WebSocket handler).

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?

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.

RX14 · 2018-03-06T00:36:40Z

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

straight-shoota · 2018-03-06T13:08:56Z

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

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).

ysbaddaden · 2018-03-06T14:35:47Z

src/http/server.cr

+  # 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


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.

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?

I prefer to keep everything local-addressed by default, and force people to choose possibly insecure defaults (0.0.0.0, ::) by conscious decision.

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.

Maybe we could have a method to listen on 0.0.0.0 and ::, like #bind_all(port : Int32)

ysbaddaden · 2018-03-06T14:44:22Z

src/http/server.cr

+  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)


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)

ysbaddaden · 2018-03-06T14:46:50Z

src/http/server.cr

+  # server = HTTP::Server.new { }
+  # server.bind Socket::UNIXAddress.new("/tmp/my-socket")
+  # ```
+  def bind(address : Socket::UNIXAddress) : UNIXServer


Are the XAddress overloads really useful?

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.

ysbaddaden · 2018-03-06T14:47:48Z

src/http/server.cr

+  end
+
+  # Adds a `Socket::Server` *interface* to this server.
+  def bind(interface)


There should be a #bind(interface : Socket::Server) type constraint.

ysbaddaden · 2018-03-06T14:51:08Z

src/http/server.cr

+
+  # Binds to the specified arguments and starts the server.
+  #
+  # See `#bind` and `#listen` for details


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).

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.

ysbaddaden · 2018-03-06T14:53:23Z

src/http/server.cr

-      @server = nil
+
+    @interfaces.each do |interface|
+      interface.close


Closing an interface can raise, which will prevent further interfaces from being closed.

Will wrap it in rescue. We can probably ignore any exceptions, right?

ysbaddaden · 2018-03-06T14:57:00Z

src/http/server/context.cr

    getter response : Response

+    # An optional `Socket` which holds the client connection of this context.
+    getter socket : Socket?


Same here. Hijacking the IO must be achieved within a request handler or response output (see WebSocket handler).

straight-shoota · 2018-03-06T15:34:05Z

@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.

RX14 · 2018-03-07T00:45:23Z

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.

straight-shoota · 2018-03-07T11:55:42Z

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.

jhass · 2018-03-07T12:05:27Z

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

straight-shoota · 2018-03-07T12:09:07Z

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

straight-shoota · 2018-03-07T12:16:52Z

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

jhass · 2018-03-07T12:20:33Z

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

straight-shoota · 2018-03-07T12:25:27Z

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).

straight-shoota · 2018-03-07T15:57:20Z

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

straight-shoota · 2018-03-14T15:17:03Z

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.

straight-shoota · 2018-03-15T11:11:52Z

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

ysbaddaden · 2018-03-15T11:26:36Z

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).

ysbaddaden

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).

ysbaddaden · 2018-03-15T11:43:36Z

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).

straight-shoota · 2018-03-15T12:25:05Z

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.

bcardiff · 2018-04-16T18:13:58Z

src/http/server.cr

+
+  # 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?


Maybe we should include something like "Use HTTP::Server#bind" in the error so we can guide the user a bit. WDYT?

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.

Yeap, I would say to add an ivar to track that too.

bcardiff · 2018-04-16T18:25:29Z

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.

straight-shoota · 2018-04-16T18:32:59Z

@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)

bcardiff · 2018-04-16T18:59:36Z

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.

straight-shoota · 2018-04-16T20:49:45Z

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.

bcardiff · 2018-04-16T21:55:03Z

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

Also I think that the standard exception style is without a full-stop.

RX14 · 2018-04-16T21:57:46Z

src/http/server.cr

  # 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


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.

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.

having getter? closed would be nice too.

Already in there ;)

straight-shoota · 2018-04-17T11:25:21Z

@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)

) * 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

RX14 requested changes Mar 6, 2018

View reviewed changes

straight-shoota force-pushed the jm/feature/http-server-interfaces branch 2 times, most recently from a20b633 to 91b7a9a Compare March 6, 2018 13:50

straight-shoota changed the title ~~Refactor HTTP::Server to bind to multiple addresses and support Unix sockets~~ Refactor HTTP::Server to bind to multiple addresses Mar 6, 2018

straight-shoota force-pushed the jm/feature/http-server-interfaces branch from 91b7a9a to 2612dcd Compare March 6, 2018 14:11

ysbaddaden previously requested changes Mar 6, 2018

View reviewed changes

straight-shoota force-pushed the jm/feature/http-server-interfaces branch 2 times, most recently from f88addb to 83f40d0 Compare March 6, 2018 15:56

straight-shoota force-pushed the jm/feature/http-server-interfaces branch from 83f40d0 to 3d3b92c Compare March 7, 2018 12:15

straight-shoota mentioned this pull request Mar 7, 2018

Expose connection metadata in HTTP::Server::Context #5784

Open

straight-shoota force-pushed the jm/feature/http-server-interfaces branch 2 times, most recently from c6306a4 to 013a078 Compare March 14, 2018 15:14

ysbaddaden reviewed Mar 15, 2018

View reviewed changes

straight-shoota force-pushed the jm/feature/http-server-interfaces branch from 013a078 to 9c0ea8e Compare March 15, 2018 12:08

bcardiff reviewed Apr 16, 2018

View reviewed changes

straight-shoota force-pushed the jm/feature/http-server-interfaces branch from 96544b6 to b3bd139 Compare April 16, 2018 20:45

bcardiff approved these changes Apr 16, 2018

View reviewed changes

RX14 requested changes Apr 16, 2018

View reviewed changes

Add HTTP::Server.listening? and raise errors when running or closed

8e17445

straight-shoota force-pushed the jm/feature/http-server-interfaces branch from b3bd139 to 8e17445 Compare April 17, 2018 08:10

RX14 approved these changes Apr 17, 2018

View reviewed changes

RX14 added the kind:feature label Apr 17, 2018

RX14 added this to the Next milestone Apr 17, 2018

RX14 merged commit 01a5126 into crystal-lang:master Apr 17, 2018

straight-shoota deleted the jm/feature/http-server-interfaces branch April 17, 2018 11:15

This was referenced Apr 17, 2018

Add HTTP::Server#bind_unix #5959

Merged

Add OpenSSL::SSL::Server and HTTP::Server#bind_ssl #5960

Merged

Sija mentioned this pull request Apr 27, 2018

Improve WebSocket handshake validation #5327

Merged

straight-shoota mentioned this pull request Apr 28, 2018

Improve WebSocket handshake validation (fixed) #6027

Merged

bcardiff mentioned this pull request May 23, 2018

Updates to upcoming Crystal 0.25.0 kemalcr/kemal#451

Merged

bew mentioned this pull request Jun 4, 2018

Change server sample of Crystal 0.25 crystal-lang/crystal-website#63

Merged

straight-shoota mentioned this pull request Jun 11, 2018

Fix docs for new HTTP::Server API #6179

Merged

Daniel-Worrall mentioned this pull request Jun 22, 2018

Fix HTTP::Server example in Playground About #6240

Closed

straight-shoota mentioned this pull request Aug 7, 2018

Add HTTP::Server#bind(URI) #6500

Merged

This was referenced Jan 2, 2019

Remove HTTP::RequestProcessor #7248

Open

Improve HTTP::Server docs #7251

Merged

straight-shoota mentioned this pull request Feb 14, 2023

HTTP::Server vs ApplicationServer #13072

Open

Uh oh!

Conversation

straight-shoota commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RX14 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RX14 Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RX14 commented Mar 6, 2018

Uh oh!

straight-shoota commented Mar 6, 2018

Uh oh!

ysbaddaden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

straight-shoota Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

straight-shoota Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

straight-shoota commented Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RX14 commented Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

straight-shoota commented Mar 5, 2018 •

edited

Loading

RX14 Mar 7, 2018 •

edited

Loading

straight-shoota Mar 6, 2018 •

edited

Loading

straight-shoota Mar 6, 2018 •

edited

Loading

straight-shoota commented Mar 6, 2018 •

edited

Loading

RX14 commented Mar 7, 2018 •

edited

Loading

straight-shoota commented Mar 14, 2018 •

edited

Loading

ysbaddaden left a comment •

edited

Loading

ysbaddaden commented Mar 15, 2018 •

edited

Loading

straight-shoota commented Mar 15, 2018 •

edited

Loading