async_hooks initial implementation by trevnorris · Pull Request #8531 · nodejs/node

trevnorris · 2016-09-14T10:59:17Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

async_wrap, async_hooks

Description of change

This is the initial implementation of the async_hooks API as outlined in nodejs/node-eps#18. There was a more extensive API initially proposed, but after completing all those features I found it had substantial performance impact. So I limited the scope of the proposal (the EP has just been updated to reflect those changes) and implemented them. I left the feature complete implementation in its own commit, and stripped it down in a following commit. So that reviewers could see how it was being done and possibly have some suggestions about how to improve that performance hit.

The biggest hit comes from tracking process.nextTick() calls. Here's a minimal http server with hooks:

const print = process._rawDebug;

require('async_hooks').createHook({
  init: (id, type) => print('init:   ', id, type),
  before: (id) =>     print('before: ', id),
  after: (id) =>      print('after:  ', id),
  destroy: (id) =>    print('destroy:', id),
}).enable();

http.createServer((req, res) => {
  res.end('bye');
}).listen(8080);

And here is the async call stack for a single request completion:

init:    21 TCPWRAP
before:  2
init:    22 TickObject
after:   2
before:  22
after:   22
destroy: 22
before:  6
init:    23 TIMERWRAP
init:    24 TickObject
init:    25 TickObject
after:   6
before:  6
init:    26 TickObject
after:   6
before:  6
after:   6
before:  24
init:    27 TickObject
after:   24
destroy: 24
before:  25
after:   25
destroy: 25
before:  26
init:    28 TickObject
after:   26
destroy: 26
before:  27
init:    29 TickObject
init:    30 TickObject
init:    31 TickObject
after:   27
destroy: 27
before:  28
after:   28
destroy: 28
before:  29
after:   29
destroy: 29
before:  30
after:   30
destroy: 30
before:  31
after:   31
destroy: 31
before:  21
init:    32 TickObject
init:    33 TickObject
after:   21
before:  32
after:   32
destroy: 32
before:  33
after:   33
destroy: 33
before:  21
after:   21
destroy: 21
before:  23
after:   23
destroy: 23

As you can see, the TickObject calls dominate the stack. So even doing something as minimal as generating a new id for each TickObject can be seen when stressing out a process.

The biggest functionality hit that was taken is that the hooks don't propagate with the call stack anymore. Instead once you run disable() they are removed from the global pool of hooks to process, and won't be run again until the user runs enable().

Would appreciate any feedback on how to reintroduce what was removed. Though even stripped down I'm seeing a few percent performance drop w/o using any callbacks. My goal was to have zero (or maybe better said is indistinguishable) overhead if the API wasn't being used. IMO that should be the first thing addressed.

Known Issues

This PR still has some bugs to fix. Here is a list:

HTTPParser doesn't propagate parent id properly
Reused resources aren't reassigned a new UID
Have potential for leak if references are made to handles that require GC for destroy() call

Fishrock123 · 2016-09-14T16:55:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/4048/

Fishrock123 · 2016-09-14T17:01:11Z

cc @nodejs/ctc & @AndreasMadsen

jasnell

Looking very good so far. Left a couple minor comments but otherwise this is +1. Will have to do a more detailed review later on.

jasnell · 2016-09-15T17:11:11Z

lib/async_hooks.js

Ugh. Can we please consider making this a constant in os.constants rather than a method?

It's been a method for a long time, I see little reason to get rid of it.

jasnell · 2016-09-15T17:13:06Z

lib/async_hooks.js

Given that there's not a clear concept of unsigned integers in js, perhaps this should say that it must be a non-negative integer?

Are you specifically referring to the bit that's interpreted differently depending on whether it's unsigned or not? I am using the definition of

capable of representing only non-negative integers

I'll change the wording if you like, but personally don't think it's necessary.

AndreasMadsen

Looks really good, but it's very tough to review.

I think most issues are documented in the TODOs, I found only one bug. Besides that:

I think you should create an async_hooks binding. Having the current_id, async_id, etc. counters in async_wrap seams to only be for historic reasons.
It appears that the C++ Embedder API hasn't been implemented.
Also if it helps, you herby have permission to steal tests from my async_hook module.

AndreasMadsen · 2016-09-15T12:26:27Z

lib/async_hooks.js

Just in case the id do get too high it might be a good idea to say it must be an unsigned safe integer. Otherwise it will be confusing when 9007199254740992 throw an error.

AndreasMadsen · 2016-09-15T12:34:01Z

lib/async_hooks.js

I haven't checked, but I would assume that checking active_hooks_array[i][init_symbol] !== undefined is faster than looking up the type.

AndreasMadsen · 2016-09-15T12:38:03Z

lib/async_hooks.js

I understand there are performance reasons for not checking with isSafeInteger. But I find it just as likely that I would pass a wrong argument to emitBefore as I would pass a wrong argument to emitInit.

AndreasMadsen · 2016-09-15T12:39:53Z

lib/async_hooks.js

Yeah, what is prevId about? It is not even used in emitAfter. I think you removed it from the documentation too.

AndreasMadsen · 2016-09-15T12:41:46Z

lib/async_hooks.js

We should document that hook.disable().enable() can change the order.

AndreasMadsen · 2016-09-15T13:14:37Z

lib/timers.js

hooks stack

Can you elaborate on what the hooks stack is.

AndreasMadsen · 2016-09-15T18:59:12Z

src/async-wrap.cc

Just want to make sure you didn't forget this.

AndreasMadsen · 2016-09-15T19:08:31Z

src/async-wrap.cc

Should be Environment::AsyncHooks::kAfter

kinda happy I found an error, halfway through I was starting to think it was perfect :)

AndreasMadsen · 2016-09-15T19:12:59Z

src/async-wrap.cc

I'm not so strong on my C++ V8, what does this mean? I assume it is similar to:

if (ret.IsEmpty()) { return ret; }

but before that was checked/called after the post hook.

AndreasMadsen · 2016-09-15T19:27:01Z

src/node.cc

How is before and after called now. The propused API is that MakeCallback(..., async_id, ...) will call before and after but this doesn't appear to do that.

Fishrock123 · 2016-09-15T22:12:37Z

lib/timers.js

Maybe just define this in the while where the code runs?

Fishrock123

Seems good, left some timers review.

(Hopefully I'm using this new review thing correctly?)

Fishrock123 · 2016-09-15T22:13:27Z

lib/timers.js

This should actually be in insert() I think.

Note: some sort of ID retention needs to be done for intervals...

(And maybe even timers that are re-inserted...)

Fishrock123 · 2016-09-15T22:13:58Z

lib/timers.js

This should be in unenroll().

Fishrock123 · 2016-09-15T22:14:33Z

lib/timers.js

Same loop/variable comment as in listOnTimeout()

Fishrock123 · 2016-09-15T22:18:29Z

lib/internal/bootstrap_node.js

Is this comment still valid since after() no longer has a didThrow?

joshgav · 2016-09-16T16:28:43Z

/cc @nodejs/diagnostics @digitalinfinity

Fishrock123

Trying to get dprof running properly on this. Progress is good, but representing timers properly with this is... difficult.

I think destory() may need to have an extra parameter for resources that may be reused in the future.

For example, someone may unenroll a non-repeating timer, keep it alive, and re-enroll it later.

A quicker example of that would be any timer that gets its duration updated.

Fishrock123 · 2016-09-18T01:15:23Z

lib/timers.js

also needs roughly this:

if (!timer._repeat && async_hook_fields[kDestroy]) async_hooks.emitDestroy(timer._asyncId);

Fishrock123 · 2016-09-18T01:17:06Z

lib/timers.js

Note: some sort of ID retention needs to be done for intervals...

(And maybe even timers that are re-inserted...)

Qard · 2016-09-18T01:58:58Z

@Fishrock123 Do you think it'd be more generically solvable with this? nodejs/node-eps#18 (comment)

Fishrock123 · 2016-09-18T02:24:43Z

@Qard hmmmm, could be. Wouldn't you also need an unqueue or finish then?

That could solve the TIMERWRAP - Timeout disparity though. Edit2: Maybe?

For example, take interval.dprof.json and look at the rendered version by:

npm i -g dprof
cat interval.dprof.json | dprof

(Should work with latest published dprof I think.)

Edit: the code is this, for reference:

setInterval(function timeout() {
  console.log('#')
}, 100)

Qard · 2016-09-18T02:33:12Z

Maybe something like this for cancelables:

create -- resource created
- queue -- request queued
    - before -- request reaches callback
       - after -- request completes callback
- queue -- request queued
    - unqueue -- request cancelled
destroy -- resource cleaned up

Fishrock123 · 2016-09-18T15:01:32Z

lib/timers.js

also needs

if (immediate._asyncId && async_hook_fields[kDestroy]) { async_hooks.emitDestroy(immediate._asyncId); }

Fishrock123 · 2016-09-18T15:47:16Z

Still needs even more timers fixes. A diff will follow this.

(From AndreasMadsen/dprof#17 (comment))

Take, for example, this.

The handles with the line in-between are actually the same, the timeout was created and the updated three times, from different places, causing re-insertion.

UIDs at the current time (with patches for timers): 12, 17, 20, then 26.

This was taken using a WIP dprof using the following patches to get roughly correct timers behavior but without conceptual same-ID re-use patching.

diff --git a/lib/timers.js b/lib/timers.js
index 6ead9c2..3194ab4 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -145,6 +145,15 @@ function insert(item, unrefed) {
     list._timer[kOnTimeout] = listOnTimeout;
   }

+  if (unrefed === true) item._timerUnref = true;
+
+  if (!item._asyncId) {
+    // No UID, assign a new one.
+    item._asyncId = async_hooks.newUid();
+    if (async_hook_fields[kInit])
+      async_hooks.emitInit(item._asyncId, item, 'Timeout');
+  }
+
   L.append(list, item);
   assert(!L.isEmpty(list)); // list is not empty
 }
@@ -188,7 +197,14 @@ function listOnTimeout() {
     L.remove(timer);
     assert(timer !== L.peek(list));

-    if (!timer._onTimeout) continue;
+    if (!timer._onTimeout) {
+      if (timer._asyncId && async_hook_fields[kDestroy]) {
+        async_hooks.emitDestroy(timer._asyncId);
+        timer._asyncId = 0
+      }
+
+      continue;
+    }

     var domain = timer.domain;
     if (domain) {
@@ -224,6 +240,10 @@ function listOnTimeout() {
       async_hooks.setCurrentId(prev_id);
       if (async_hook_fields[kAfter])
         async_hooks.emitAfter(timer._asyncId);
+      if (!timer._repeat && async_hook_fields[kDestroy]) {
+        async_hooks.emitDestroy(timer._asyncId);
+        timer._asyncId = 0
+      }
     }

     if (domain)
@@ -300,6 +320,12 @@ function reuse(item) {

 // Remove a timer. Cancels the timeout and resets the relevant timer properties.
 const unenroll = exports.unenroll = function(item) {
+  if (item._asyncId) {
+    if (async_hook_fields[kDestroy])
+      async_hooks.emitDestroy(item._asyncId);
+    item._asyncId = 0;
+  }
+
   var handle = reuse(item);
   if (handle) {
     debug('unenroll: list empty');
@@ -480,9 +506,7 @@ function Timeout(after) {
   this._idleStart = null;
   this._onTimeout = null;
   this._repeat = null;
-  this._asyncId = async_hooks.newUid();
-  if (async_hook_fields[kInit])
-    async_hooks.emitInit(this._asyncId, this, 'Timeout');
+  this._asyncId = 0;
 }


@@ -534,8 +558,6 @@ Timeout.prototype.close = function() {
   } else {
     unenroll(this);
   }
-  if (this._asyncId && async_hook_fields[kDestroy])
-    async_hooks.emitDestroy(this._asyncId);
   return this;
 };

@@ -575,6 +597,10 @@ function processImmediate() {
         async_hooks.emitAfter(immediate._asyncId);
     }

+    if (immediate._asyncId && async_hook_fields[kDestroy]) {
+      async_hooks.emitDestroy(immediate._asyncId);
+    }
+
     if (domain)
       domain.exit();
   }

Fishrock123 · 2016-09-19T14:04:49Z

Perhaps it is better to just re-use the same UID and forget about re-insertion pre- timeout/unenroll.

That is, only destroy and re-insert after timeout or removal, not on updating the timeout duration or start (re-insert.)

e.g. this patch on the last one:

diff --git a/lib/timers.js b/lib/timers.js
index 9f397cd..3194ab4 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -152,13 +152,6 @@ function insert(item, unrefed) {
     item._asyncId = async_hooks.newUid();
     if (async_hook_fields[kInit])
       async_hooks.emitInit(item._asyncId, item, 'Timeout');
-  } else if (!item._repeat) {
-    // UID but no repeat, emit Destroy on the existing, then create a new one.
-    if (async_hook_fields[kDestroy])
-      async_hooks.emitDestroy(item._asyncId);
-    item._asyncId = async_hooks.newUid();
-    if (async_hook_fields[kInit])
-      async_hooks.emitInit(item._asyncId, item, 'Timeout');
   }

   L.append(list, item);
@@ -205,8 +198,10 @@ function listOnTimeout() {
     assert(timer !== L.peek(list));

     if (!timer._onTimeout) {
-      if (timer._asyncId && async_hook_fields[kDestroy])
+      if (timer._asyncId && async_hook_fields[kDestroy]) {
         async_hooks.emitDestroy(timer._asyncId);
+        timer._asyncId = 0
+      }

       continue;
     }
@@ -245,8 +240,10 @@ function listOnTimeout() {
       async_hooks.setCurrentId(prev_id);
       if (async_hook_fields[kAfter])
         async_hooks.emitAfter(timer._asyncId);
-      if (!timer._repeat && async_hook_fields[kDestroy])
+      if (!timer._repeat && async_hook_fields[kDestroy]) {
         async_hooks.emitDestroy(timer._asyncId);
+        timer._asyncId = 0
+      }
     }

     if (domain)

Gives this output: (Same handle circled in orange, UID 12.)

Fishrock123 · 2016-09-19T14:07:38Z

It may be worthwhile to have timers use their own unique _asyncId property though, since timers can be made off of other objects that could have an _asyncId it could get very confusing...

e.g. _timerAsyncId

Edit: or we could just use symbol-hidden properties.

trevnorris · 2016-09-20T18:18:32Z

@Fishrock123

Perhaps it is better to just re-use the same UID and forget about re-insertion pre- timeout/unenroll.

That is, only destroy and re-insert after timeout or removal, not on updating the timeout duration or start (re-insert.)

agreed.

It may be worthwhile to have timers use their own unique _asyncId property though, since timers can be made off of other objects that could have an _asyncId it could get very confusing...

e.g. _timerAsyncId

to clarify, you mean to use a different property name and not to have their own unique id mechanism? that works for me.

Fishrock123 · 2016-09-20T19:45:53Z

to clarify, you mean to use a different property name and not to have their own unique id mechanism? that works for me.

I mean a different property name for the ID specific to timers so that it does not conflict because any object could be a timer. (yes, I think...?)

Add NODE_TEST_WITH_ASYNC_HOOKS environment variable to allow running tests with init/before/after/destroy noops to make sure that basic mechanics work properly across all tests.

Place restoreTmpHooks() closer to setupTmpActiveHooks() to make it more apparent what each is used for.

Switch to using a v8::Eternal array to retrieve the provider strings passed as the "type" to the init() callback.

Instead of always passing in a Float64Array to getIdsFromFd() preallocate one during initialization and write to those fields directly.

When emitAfterS() is called, make sure the id passed in is the same id that's on the stack.

destroy() hooks from JS should also be called async, but instead of trying to do this in a setImmediate, just use the same list that is used for native ids.

Add NODE_TEST_HANDLE_ACCESS, which tests if a resource aborts if inspected at the end of the process (specifically if the resource has been cleaned up on the C++ side), and NODE_CHECK_ASYNC_DESTROY, which checks if destroy has been called on the same id twice.

This was originally done as a safety measure, but fails in practice if a clear*() is run on the same handle that's currently executing its callback.

In some cases, like triggering the microtask queue, there is no id (though that should change in the future). So instead trigger that execution is happening in the void, and return early if destroy is emitted on the void.

async_hooks does not allow recovery from errors because it's currently unknown what will happen to the state of the application if that happens. So instead, force the application to print an error message then either exit or abort. Depending on whether the user passed --abort-on-uncaught-exception. exit() is used instead of reallyExit() to allow any 'exit' events to fire.

ForceSet() everything with v8::ReadOnly | v8::DontDelete to make sure none of the object properties can be overridden.

In order to get a stack tracker that is performant in JS had to do some ridiculous stuff. This allows the stack to be tracked from both C++ and JS with almost no overhead, and allows it to unwind properly in case there's a fatal exception.

Instead of using a CHECK(), print an error message to the user. Check if --abort-on-uncaught-exception has been passed, and abort if this is the case. Otherwise exit(1).

trevnorris · 2017-03-16T17:54:52Z

Superseded by #11883. Closing.

bajtos · 2017-03-27T09:49:59Z

Superseded by #11883. Closing.

@trevnorris would you mind updating the checklist in nodejs/diagnostics#29 with the new PR number?

setTimeout, setInterval, setImmediate (old issue: async-wrap: nextTick and setImmediate is ignored #666, PR: async_hooks initial implementation #8531)

AndreasMadsen · 2017-03-27T10:26:49Z

@bajtos Thanks, I've just done that.

trevnorris added the async_wrap label Sep 14, 2016

rvagg mentioned this pull request Sep 15, 2016

AsyncWrap public API proposal nodejs/node-eps#18

Merged

jasnell reviewed Sep 15, 2016

View reviewed changes

AndreasMadsen reviewed Sep 15, 2016

View reviewed changes

Fishrock123 reviewed Sep 15, 2016

View reviewed changes

lib/timers.js Outdated

Copy link

Contributor

Fishrock123 Sep 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just define this in the while where the code runs?

Fishrock123 suggested changes Sep 15, 2016

View reviewed changes

Fishrock123 mentioned this pull request Sep 17, 2016

test: Refactors test/parallel/test-async-wrap-post-did-throw.js #8625

Closed

1 task

Fishrock123 suggested changes Sep 18, 2016

View reviewed changes

This was referenced Sep 18, 2016

Rewrite to use actual async_hooks AndreasMadsen/dprof#17

Closed

current uid is a stack AndreasMadsen/dprof#10

Closed

Fishrock123 suggested changes Sep 18, 2016

View reviewed changes

Fishrock123 mentioned this pull request Sep 19, 2016

[refactor] initial rewrite to use native async_hooks AndreasMadsen/dprof#19

Merged

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09

trevnorris added 16 commits January 27, 2017 10:44

test: allow running with async_hook noops

bd660a4

Add NODE_TEST_WITH_ASYNC_HOOKS environment variable to allow running tests with init/before/after/destroy noops to make sure that basic mechanics work properly across all tests.

async_hooks: move location of restoreTmpHooks()

e8c27f8

Place restoreTmpHooks() closer to setupTmpActiveHooks() to make it more apparent what each is used for.

async_wrap: use v8::Eternal for provider strings

0ab9547

Switch to using a v8::Eternal array to retrieve the provider strings passed as the "type" to the init() callback.

fs: use preallocated Float64Array

352c89a

Instead of always passing in a Float64Array to getIdsFromFd() preallocate one during initialization and write to those fields directly.

PARTIAL remove all applicable TODOs

e9ae576

async_hooks: verify stack integrity

015e665

When emitAfterS() is called, make sure the id passed in is the same id that's on the stack.

async_hooks: place destroy ids on native list

e2af82f

destroy() hooks from JS should also be called async, but instead of trying to do this in a setImmediate, just use the same list that is used for native ids.

timers: don't reset _asyncId/_triggerId

ca8d6db

This was originally done as a safety measure, but fails in practice if a clear*() is run on the same handle that's currently executing its callback.

async_hooks: return early if emitting the void

ac2e137

In some cases, like triggering the microtask queue, there is no id (though that should change in the future). So instead trigger that execution is happening in the void, and return early if destroy is emitted on the void.

async_wrap: don't allow exports to be overwritten

25dd8af

ForceSet() everything with v8::ReadOnly | v8::DontDelete to make sure none of the object properties can be overridden.

fix bug in array buffer size in fs

2d98170

PARTIAL implement new stack tracker

7bc7d3c

In order to get a stack tracker that is performant in JS had to do some ridiculous stuff. This allows the stack to be tracked from both C++ and JS with almost no overhead, and allows it to unwind properly in case there's a fatal exception.

timers: don't emit destroy() on same id twice

edf67a7

test: make async hooks test better

90f0a63

targos removed the dont-land-on-v7.x label Jan 28, 2017

joyeecheung mentioned this pull request Jan 31, 2017

discuss: future of AsyncWrap #7565

Closed

trevnorris force-pushed the async-wrap-eps-impl branch from 97a27f1 to 79b11ce Compare February 1, 2017 06:13

async_wrap: print msg on error instead of abort

1ff2dfb

Instead of using a CHECK(), print an error message to the user. Check if --abort-on-uncaught-exception has been passed, and abort if this is the case. Otherwise exit(1).

trevnorris force-pushed the async-wrap-eps-impl branch from 79b11ce to 1ff2dfb Compare February 2, 2017 00:06

LewisJEllis mentioned this pull request Feb 4, 2017

Domain Module Deprecated getsentry/raven-node#264

Open

joshgav added the diag-agenda Issues and PRs to discuss during the meetings of the diagnostics working group. label Feb 23, 2017

joshgav mentioned this pull request Feb 23, 2017

Diag WG Meeting - 2017-02 nodejs/diagnostics#85

Closed

joshgav removed the diag-agenda Issues and PRs to discuss during the meetings of the diagnostics working group. label Feb 23, 2017

trevnorris mentioned this pull request Apr 4, 2017

async_hooks initial implementation #11883

Closed

4 tasks

AndreasMadsen mentioned this pull request May 8, 2017

Async Hooks - Initial Implementation #12892

Closed

4 tasks

Uh oh!

Conversation

trevnorris commented Sep 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Known Issues

Uh oh!

Fishrock123 commented Sep 14, 2016

Uh oh!

Fishrock123 commented Sep 14, 2016

Uh oh!

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

AndreasMadsen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndreasMadsen Sep 15, 2016 • 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!

AndreasMadsen Sep 15, 2016 • 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!

AndreasMadsen Sep 15, 2016 • 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!

AndreasMadsen Sep 15, 2016 • 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!

AndreasMadsen Sep 15, 2016 • 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!

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

joshgav commented Sep 16, 2016

Uh oh!

Fishrock123 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

trevnorris commented Sep 14, 2016 •

edited

Loading

AndreasMadsen Sep 15, 2016 •

edited

Loading

AndreasMadsen Sep 15, 2016 •

edited

Loading

AndreasMadsen Sep 15, 2016 •

edited

Loading

AndreasMadsen Sep 15, 2016 •

edited

Loading

AndreasMadsen Sep 15, 2016 •

edited

Loading

Fishrock123 commented Sep 18, 2016 •

edited

Loading

Fishrock123 Sep 18, 2016 •

edited

Loading

Fishrock123 commented Sep 18, 2016 •

edited

Loading

Fishrock123 commented Sep 19, 2016 •

edited

Loading

Fishrock123 commented Sep 20, 2016 •

edited

Loading