Rewrite ConnectionReuse #387

gnunicorn · 2018-08-02T15:25:13Z

based upon #355 but has a centralised ConnectionManager, which encapsulates and manages all state transitions in a few central places and ensures everyone is notified as needed if those occur. Which means:

No one outside of it knows about the PeerState-Enum whatsoever
Everyone just references the Arc not the Mutex, the Managers manages all locking
Independent locks for connections and counters
More specific locks around the connections with less code and more explicit transitions, making it easier to reason about the code
Ensures Counters are increased properly (rather than relying on external parties to do that)
No Poisoned-State possible anymore (and removed alltogether)
Remove connection_id-system and consequently all referencing of ConnectionManager in Streams, replace with shared closed-state to stop streams early
Add reset_connection on ConnectionReuseSubstream to allow a substream to state that a connection is to be considered closed (primarily for the Ping-protocol) and doesn't continue producing
which all allowed removing many unnecessary fields of PeerState

Further more:

remove substream-referencing of ConnectionManager alltogether, massivley reduxoing
[Performance] Replaced FnvHashMap<usize, task::Task>, with a faster Vec<(usize, task::Task)>
[Fix] When an incoming connection replaced a pending state, we didn't inform all listeners, we now close all streams
[Fix] reliably ensure we aren't keeping around/use old potentially stale connections, fixes a leak and potential DoS-Attack-Window.

twittner

Please note that #355 also addresses #344 by tracking the number of streams per connection, removing it when reaching 0.

twittner · 2018-08-07T16:25:31Z

core/src/connection_reuse.rs

+    fn reset(&self, addr: &Multiaddr) {
+        let mut conns = self.connections.lock();
+        self.reset_conn(conns.remove(&addr));
+    }


reset is never used.

twittner · 2018-08-07T16:27:47Z

core/src/connection_reuse.rs

+{
+    /// Informs the Manager about a new inbound connection that has been
+    /// established, so it
+    fn new_inbound(&self, addr: Multiaddr, listener_id: usize, muxer: C::Output) {


Why is addr not taken by reference?

twittner · 2018-08-07T16:28:45Z

core/src/connection_reuse.rs

+    /// the connection isn't establed and ready yet.
+    fn poll_outbound(
+        &self,
+        addr: Multiaddr,


Why is addr not taken by reference?

twittner · 2018-08-08T08:29:58Z

core/src/connection_reuse.rs

-    // Struct shared between most of the `ConnectionReuse` infrastructure.
-    shared: Arc<Mutex<Shared<C::Output>>>,
+    /// Tasks to notify when one or more new elements were added to `connections`.
+    notify_on_new_connec: Mutex<Vec<(usize, task::Task)>>,


Instead of a Vec O(n) why not a HashMap O(1)?

We were wondering about the performance of this, too so I made a bunch of test. They showed: The overhead of hashing is only cut of once the vec is actually quite large (>200) - which is super unlikely to happen for the notifications (as barely ever more than a handfull of tasks would want to be notified, and we drain it every time we do).

What makes HashMap even worse in this scenario, is that in practice we intend-to-insert much more often than we actually insert, and every time, we have to do the expensive hashing in the hashmap while we'd exit early in looping through the vector. In practice the vector has one or two entries, making it faster to loop than to hash (even with Fnv).

I was told these supposedly tiny performance differences matter, so I made the change.

I do not think these benchmarks do the same thing. For instance bench_custom_hasher_hashmap_same does COUNT inserts, whereas bench_with_vec_same returns early, i.e. after MODULO inserts. Changing bench_with_vec_same to do (roughly) what the HashMap based benchmark does

#[bench] fn bench_with_vec_same(b: &mut Bencher) { b.iter(|| { let n = test::black_box(COUNT); let mut v: Vec<(Key, &'static str)> = vec![]; 'outer: for idx in 0..n { let id = idx % MODULO; for i in 0..v.len() { if v.get(i).unwrap().0 == id { v[i] = (id, "value"); continue 'outer; } } v.push((id, "value")); }; }); }

yields

running 2 tests test bench_custom_hasher_hashmap ... bench: 8,800 ns/iter (+/- 376) test bench_with_vec_same ... bench: 32,970 ns/iter (+/- 1,188)

@twittner oh, yes, what a stupid mistake - I had an iterator in there first and never replaced that return. I have to run these tests again.

why are you doing a v[i] = (id, "value"); if you found it though?

Just to mimick HashMap::insert which also updates existing entries.

Sorry about that, that was a stupid bug. However, I inserted the Modulo only to emulate a scenario closer to our "many attempts little inserts" scenario. I only tried that because the worst-case-scenario (never matches, inserts every time) yielded good results for smaller sizes, which was unaffected by the bug. I fixed the code and running it now on small sets still yields significant performance improvemnets. On my system - take it with a grain of salt, flexible cpu-rate and all - the HashMap only shows benefit after 30+ items, which we'll probably not reach in practice:

# 500 Modulo 30 test bench_custom_hasher_hashmap_same ... bench: 5,626 ns/iter (+/- 608) test bench_fnv_hashmap_same ... bench: 8,043 ns/iter (+/- 3,692) test bench_fnv_hashset_same ... bench: 6,652 ns/iter (+/- 394) test bench_fnv_hashset_uniques ... bench: 12,490 ns/iter (+/- 2,910) test bench_with_sorted_vec_same ... bench: 11,130 ns/iter (+/- 457) test bench_with_vec_same ... bench: 6,454 ns/iter (+/- 154) test result: ok. 0 passed; 0 failed; 0 ignored; 6 measured; 0 filtered out

(the custom hasher shows unreliable results for me :( )

I can not reproduce those results. bench_custom_hasher_hashmap_same is always faster or on par with bench_with_vec_same on my machine, so I would prefer to go with Big-O and use a HashMap.

# 100 Modulo 20 running 2 tests test bench_custom_hasher_hashmap_same ... bench: 909 ns/iter (+/- 62) test bench_with_vec_same ... bench: 1,026 ns/iter (+/- 59) # 200 Modulo 20 running 2 tests test bench_custom_hasher_hashmap_same ... bench: 1,782 ns/iter (+/- 74) test bench_with_vec_same ... bench: 1,903 ns/iter (+/- 93) # 2000 Modulo 20 running 2 tests test bench_custom_hasher_hashmap_same ... bench: 17,063 ns/iter (+/- 366) test bench_with_vec_same ... bench: 17,590 ns/iter (+/- 933) # 1000 Modulo 100 running 2 tests test bench_custom_hasher_hashmap_same ... bench: 9,126 ns/iter (+/- 448) test bench_with_vec_same ... bench: 38,695 ns/iter (+/- 1,522) # 500 Modulo 30 running 2 tests test bench_custom_hasher_hashmap_same ... bench: 4,605 ns/iter (+/- 237) test bench_with_vec_same ... bench: 7,246 ns/iter (+/- 471)

@twittner and then use the custom hasher? Fine with me. I am not sure my results aren't influenced because of the automatic CPU-frequency-govener that I can't disable :( . Especially for the first tests this probably skewes the result as it will only ramp up CPU freq during the test... That the custom hasher (which does nothing) is supposedly magnitutes slower than the Fnv supports that thesis - because Fnv doesn't do anything else than we do...

I'll do that.

I think SmallVec should be faster because of cache locality.

twittner · 2018-08-08T08:34:35Z

core/src/connection_reuse.rs

+        /// Future that produces the muxer.
+        future: Box<Future<Item = (M, Multiaddr), Error = IoError>>,
+        /// All the tasks to notify when `future` resolves.
+        notify: Vec<(usize, task::Task)>,


Instead of a Vec O(n) why not a HashMap O(1)?

twittner · 2018-08-08T09:16:54Z

core/src/connection_reuse.rs

+    /// substreams
+    fn reset_conn(&self, state: Option<PeerState<C::Output>>) {
+        if let Some(PeerState::Active { closed, .. }) = state {
+            closed.store(false, Ordering::Relaxed);


I do not think Relaxed is correct. When loading the value elsewhere there are no ordering guarantees, therefore this store may not become visible to subsequent loads.

twittner · 2018-08-08T09:28:20Z

core/src/connection_reuse.rs


-        // Nothing is ready.
-        Ok(Async::NotReady)
+        self.inner.read(buf)  


Trailing whitespace.

twittner · 2018-08-08T09:43:10Z

core/src/connection_reuse.rs

+                    }
+                    continue;
+                }
+            };


Personally I would have found a match easier to read.

twittner · 2018-08-08T09:55:19Z

core/src/connection_reuse.rs

+            }
+            Err(err) => Err(err),
+            Ok(Async::NotReady) => Ok(Async::NotReady)
+        }


This match could be simpified with try_ready!.

twittner · 2018-08-08T10:13:31Z

core/src/connection_reuse.rs

 use tokio_io::{AsyncRead, AsyncWrite};
 use transport::{MuxedTransport, Transport, UpgradedNode};
 use upgrade::ConnectionUpgrade;
+use std::clone::Clone;


Unnecessary import.

gnunicorn · 2018-08-08T14:48:29Z

@twittner yes, indeed, it intended to. However, it has/had a bug and a major drawback:

when a new connection came in, it would just overwrite any potentially existing PeerState-Entry with a new one - if that was an active one, it would leaving all existing connections dangling. Given, it is a bit weird they connect if we are already connected, but then, they might do it because the connection timed and we just don't know yet. While this also contains a potential leak attack vector, in practice most streams would time out and then be dropped, decreasing the counter - a counter, we just recently reseted, and it didn't actually increase - thus leading to the connection being dropped much earlier than it should be as the counter reaches zero in the next drop. Annoying.
through that system no stream could signal the connection to forcefully reset, all you could do is drop this stream and hope that others drop their streams, too. The use case here is that the Ping-Protocol should have a way to signal that the entire connection must be dropped and no other should continue to read or write, once we detected it to be down. With the simple counter this was impossible to realise unless all streams would check that counter every time they are asked to perform an action, leading to quite a performance penalty. (In the same vain, the counter lead to a much more complex PeerState and Substream-Struct)

twittner · 2018-08-09T09:38:14Z

no stream could signal the connection to forcefully reset

Removing a connection from the cache does not necessarily close or reset the connection. Depending on the StreamMuxer implementation, each Substream may have a reference to the underlying connection. To properly implement connection resets the StreamMuxer trait would need to have a close or reset method added.

gnunicorn · 2018-08-09T12:25:33Z

@twittner

To properly implement connection resets the StreamMuxer trait would need to have a close or reset method added.

Agreed. However, with type Substream: AsyncRead + AsyncWrite; being the only thing we return to many and neither of them having close or reset functions, I added the shared closed: Arc<AtomicBool>, which anyone with access to ConnectionReuseSubstream can set through calling reset_connection on it. It will still not properly clean up the connections but rather renders them useless, so we assume they are being dropped because of that. But you are correct, this isn't the cleanest way and we might want to consider an official "closing"-functionality.

gnunicorn · 2018-08-09T14:05:31Z

@twittner addressed your concerns.

twittner

Looks good to me. Would be nice to get a review by @tomaka as well.

tomaka

IMO this code is still far too complicated.
I feel like the only difference is that before I was the only one to understand how it works, while now you are the only one to understand how it works.

That being said if you are confident that it works, then let's merge this, but we must simplify it in the future.

tomaka · 2018-08-13T09:37:40Z

core/src/connection_reuse.rs

+use std::hash::{BuildHasherDefault, Hasher};
+
+#[derive(Default)]
+struct UsizeHasher {


Why not use FnvHashMap, which afaik does this better, instead of defining our own hasher?

Performance. See the conversation I had with @twittner about this. I am not sure what you mean with FnvHashMap doing "this better", FnvHasher does the same we do here: it defines its own hasher, using the Fnv-Algo. However as we already have pretty unique values, that Algo is a bit more expensive (~10-15% in this case) than us just converting the usize-Key into the required u64 type. We are doing the exact same as Fnv but with a less expensive "algorithm" (if you can call it that).

This algorithm however is very much constraint to the use case at hand and should probably not be re-used at many other places. Which is why I can understand the argument to go for a more generic solution - if we are willing to pay the performance price, I am fine with doing that, too.

tomaka · 2018-08-13T09:39:15Z

core/src/connection_reuse.rs

+/// Knows about all connections and their current state, allows one to poll for
+/// incoming and outbound connections, while managing all state-transitions that
+/// might occur automatically.
+// #[derive(Clone)]


Let's remove this comment. This struct doesnt' need to be cloned.

tomaka · 2018-08-13T09:40:37Z

core/src/connection_reuse.rs

+    /// substreams. Consume the PeerState while doing so.
+    fn reset_conn(self) {
+        if let PeerState::Active { closed, .. } = self {
+            closed.store(false, Ordering::Release);


I'm not a fan of having Ordering::Release without any context, as the ordering to use strongly depends on the context.
Personally I'd just copy paste these three lines of code at the two places where we use them.

I generally agree with you regarding copying three lines around. The reason I went for this being a constraint within a function rather than copied is that this isn't yet fully resetting the connection yet. So, I can see us quickly expanding what should happen here (by first adding some debugging statements, later doing some actual signaling/cleaning up of the related connections) and if you then had to do that at many code location, you easily miss one when doing changes and most reviewer(s) won't notice. If you insist, though, I don't have a strong opinion on this and can do a copy instead.

Regarding the Ordering:Release, I am not sure what else we would want to do here/at the specific locations. @twittner pointed out correctly, that Ordering:Relaxed might simply never gets propagated. I don't think that we need to expect a strong guarantee of ordering in this case, as it doesn't actually hurt if you read the remaining bytes of an already closed connection. The only important things here, to me is, that it will eventually fail and not dangle around for too long.

tomaka · 2018-08-13T09:43:53Z

core/src/connection_reuse.rs

-    active_connections: FnvHashMap<Multiaddr, M>,
+    /// Notify all `Tasks` in `self.notify_on_new_connec` that a new connection was established
+    /// consume the tasks in that process
+    fn new_con_notify(&self) {


I commented about this in private, but I'm really not a fan of locking a mutex in a function that is intended to be called from another function which can potentially lock a mutex as well.
This greatly increases the difficulty of the reviewer if you want to make sure that there is no deadlock.

I commented about this in private, but I'm really not a fan of locking a mutex in a function that is intended to be called from another function which can potentially lock a mutex as well.

While I agree on the general merit of this statement, I don't think this applies here: There are two distinct Mutexs, each one is clearly bound to corresponding function calls. I don't really see how it gives us any benefit if instead, we'd Mutex the entire ConnectionManager (what we had before) and therefore prevent anyone using the new_notify_registration (the only function actually used from the outside) until we are done with the respective poll_s. Even in that scenario, if task.1.notify would cause an immediate poll (which is the only side-effect function and we know it doesn't as this wake-up is only queued) we'd deadlock.

This greatly increases the difficulty of the reviewer if you want to make sure that there is no deadlock.

I totally agree with you that Mutexs make the review harder, as you need to make sure there isn't any side-effect code within the locked code that might cause a deadlock. But that isn't really caused by the number of Mutexs but rather how they are used and what code is executed while they are locked. Which is exactly why I try to keep the code between locks as constraint, short and side-effect-free as possible.

I am fine with moving this lock+drain on the respective position in the code base (as it is only used once) but I see little to no merit in putting the entire object into a mutex just for these two clearly independent locks... If you insist however, I can also do that.

tomaka · 2018-08-13T09:44:43Z

core/src/connection_reuse.rs

+    fn clear_error(&self, addr: Multiaddr) -> bool {
+        let mut conns = self.connections.lock();
+
+        if let Entry::Occupied(e) = conns.entry(addr) {


Should be get() instead of entry() so that clear_error can take addr by reference.

tomaka · 2018-08-13T09:45:39Z

core/src/connection_reuse.rs

+    }
+}
+
+fn poll_for_substream<M>(mut outbound: M::OutboundSubstream, addr: &Multiaddr)


What does this function do?
I don't care if a function doesn't have documentation if it is trivial, but this function clearly is not.

gnunicorn · 2018-08-13T12:01:26Z

@tomaka addressed your remarks. Simplified code, fixed or commented on.

I am not fond of this (or anything) to be only understood by one person (no matter how many tests we have). I totally agree this is still super complicated (code-wise) and I am happy to discuss how we can simplify it further. Do you have any ideas?

…dy; more traces for debugging; `iter().any()` is a more effecient approach rather than `iter().all()`

…ncoming streams

tomaka · 2018-08-13T12:05:18Z

Do you have any ideas?

Extract code into submodules.
However the splitting point is not easy to find.

gnunicorn · 2018-08-14T08:38:46Z

Decided to try a totally different approach. Pausing.

tomaka · 2018-08-31T14:05:18Z

Could you fix the conflicts?

tomaka · 2018-09-02T16:14:56Z

I updated with the changes in master.

tomaka

I don't see any code that closes the muxer when all the substreams have been closed. This has been a problem in the past, because the connection will eventually time out and we have no way to know.

I'm not really confident in merging this considering that I will likely have to debug and fix issues arising in substrate myself.

tomaka · 2018-09-02T16:16:53Z

core/src/connection_reuse.rs

+                        }
+                        let muxer = Arc::new(muxer);
+                        match poll_for_substream(muxing::outbound_from_ref_and_wrap(muxer.clone()), &addr) {
+                            Ok(None) => return Ok(Async::NotReady),


If we successfully dial a remote but it's not ready to open a substream yet, this means we will poll the dial again the next time and probably panic.

tomaka · 2018-09-02T16:18:04Z

core/src/connection_reuse.rs

+
+/// Polls the given outbound of addr for a new substream. Returns `Ok(Some(stream))` if available,
+/// `Ok(None)` if the stream is not ready yet add an `Err` if the stream had failed.
+fn poll_for_substream<O, S>(mut outbound: O, addr: &Multiaddr)


Why isn't that a Poll, so that we return Async::Ready and NotReady instead of respectively Some and None?

tomaka · 2018-09-02T16:18:33Z

core/src/connection_reuse.rs

+    <UpgradedNode<T, C> as Transport>::MultiaddrFuture: 'static,
+{
+    /// Informs the Manager about a new inbound connection that has been
+    /// established, so it


tomaka · 2018-09-02T16:20:03Z

core/src/connection_reuse.rs

+                task.notify()
+            }
+        } else {
+            old_state.map(|s| s.reset_conn());


This is very bad style in my opinion. You're mixing a "pulling" API (map) with a "pushing" action (reset_conn).

tomaka · 2018-09-02T16:22:53Z

core/src/connection_reuse.rs

+                        "Using existing connection to {} to open outbound substream",
+                        addr
+                    );
+                    match poll_for_substream(muxing::outbound_from_ref_and_wrap(muxer.clone()), &addr) {


We're creating a new outbound future, but we immediately throw it away if it's not ready? There's no way this can work for something less trivial than mplex.

tomaka · 2018-09-02T16:28:41Z

core/src/connection_reuse.rs

+                ..
+            } => {
+                if closed.load(Ordering::Acquire) {
+                    (PeerState::Errored(IoError::new(IoErrorKind::BrokenPipe, "Connection closed")),


If we open a connection, then later close it, then later attempt to open again, we will forever trigger this condition, no?
If I'm not mistaken, we should attempt to dial again instead.

tomaka · 2018-09-02T16:32:03Z

core/src/connection_reuse.rs

+            };
+            self.new_con_notify();
+            // Althought we've just started in pending state, there are transports, which are
+            // immediately ready - namely 'memory' - so we need to poll our future right away


You always need to poll immediately, otherwise the current task will not be waken up later when it needs to.

tomaka · 2018-09-02T16:33:55Z

core/src/connection_reuse.rs

+        }
+
+        for to_remove in to_remove {
+            conn.remove(&to_remove).map(|s| s.reset_conn());


I would really really really prefer if let Some over map

gnunicorn · 2018-09-03T14:37:58Z

closing in favor of #446.

ghost assigned gnunicorn Aug 2, 2018

ghost added the in progress label Aug 2, 2018

gnunicorn requested a review from tomaka August 6, 2018 07:36

gnunicorn added pr-needs-review and removed in progress labels Aug 6, 2018

ghost added the in progress label Aug 6, 2018

twittner reviewed Aug 8, 2018

View reviewed changes

gnunicorn force-pushed the connec-reuse-rewrite branch 2 times, most recently from abee6a9 to af1760a Compare August 9, 2018 13:14

gnunicorn requested a review from twittner August 10, 2018 14:33

twittner approved these changes Aug 10, 2018

View reviewed changes

tomaka suggested changes Aug 13, 2018

View reviewed changes

tomaka and others added 13 commits August 13, 2018 14:01

Rewrite ConnectionReuse

cca7c6e

Listen to incoming substreams on the listener

623230d

Fix dialer not counting as active substream

9977dfc

Fix leak

9dbf7df

Fix potentially using tons of memory

a1216df

Refactor to use Shared in a centralised manner

90325ef

Fix code style, minor changes for clarity

6fd6203

Explicit Counter

ed523df

Fully lock poll_incoming to enforce consistency

48e79d3

also move the poll_outbound into the manager

1156e02

Rename Shared -> ConnectionsManager

f8b2269

move the last external references into the manager

772ef62

remove redundant connection_id and add docs

f1095d8

gnunicorn added 16 commits August 13, 2018 14:01

Fixing tests

b87d589

extended debugging

725fc00

Docs and Code style

f8fdcfe

Also immediately check whether the incoming Pending connection is rea…

a8985ea

…dy; more traces for debugging; `iter().any()` is a more effecient approach rather than `iter().all()`

insert_with rather than insert

b6d18ba

do not immediately drop the connection, it might still have pending i…

a08bd9a

…ncoming streams

Massive Rewrite to drop stream_ids, track closed-state via AtomicBool

9f504f2

Remove unused method

567b9ed

Reference by ... well... reference

44ded6c

refactor to use match to improve readibility

a89d521

Simplify by using

f17f6cd

Replace vector of tasks with custom-hasher HashMap of tasks

38695c3

Switch from Relaxed to Release&Acquire-Ordering

4829220

move reset_con into PeerState

d7636af

addressing grumbles: clean up clear_error, remove comment

375d9d7

Simplify poll_for_substream, add docs for it

ac9eacc

gnunicorn force-pushed the connec-reuse-rewrite branch from 1758f48 to ac9eacc Compare August 13, 2018 12:02

tomaka approved these changes Aug 13, 2018

View reviewed changes

gnunicorn added on-ice and removed pr-needs-review labels Aug 14, 2018

Merge remote-tracking branch 'upstream/master' into HEAD

6e6a92b

ghost assigned tomaka Sep 2, 2018

tomaka suggested changes Sep 2, 2018

View reviewed changes

gnunicorn closed this Sep 3, 2018

ghost removed the in progress label Sep 3, 2018

Rewrite ConnectionReuse #387

Rewrite ConnectionReuse #387

Uh oh!

Conversation

gnunicorn commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

twittner 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!

gnunicorn Aug 8, 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!

gnunicorn Aug 9, 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!

Choose a reason for hiding this comment

Uh oh!

gnunicorn commented Aug 8, 2018

Uh oh!

twittner commented Aug 9, 2018

Uh oh!

gnunicorn commented Aug 9, 2018

Uh oh!

gnunicorn commented Aug 9, 2018

Uh oh!

twittner left a comment

Choose a reason for hiding this comment

Uh oh!

tomaka 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!

gnunicorn Aug 13, 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!

gnunicorn commented Aug 2, 2018 •

edited

Loading

gnunicorn Aug 8, 2018 •

edited

Loading

gnunicorn Aug 9, 2018 •

edited

Loading

gnunicorn Aug 13, 2018 •

edited

Loading