timers: fix cleanup of nested same-timeout timers #7827

erinishimoticha · 2016-07-21T19:22:34Z

Checklist

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

Affected core subsystem(s)

timers

Description of change

For nested timers with the same timeout, we can get into a situation
where we have recreated a timer list immediately before we need to
clean up an old timer list with the same key. Fix: make sure the list
to be deleted is the same instance as the list whose reference was used
to determine that a cleanup is necessary. If it's not the same instance,
a new list with the same key has been created, and it should not be
deleted.

Fixes: #7722

Fishrock123 · 2016-07-21T20:22:55Z

test/parallel/test-timers-same-timeout-wrong-list-deleted.js

+const TIMEOUT = 100;
+const start = Timer.now();
+
+function testNestedTimers() {


The contents of this function could probably just be top-level?

misterdjules · 2016-07-21T20:24:43Z

I was just about to look for other ways than twitter to reach out to people potentially interested in fixing this issue, and just found this PR. That's great :) I'll take a look asap, thank you!

Fishrock123 · 2016-07-21T20:25:29Z

Here's a CI run: https://ci.nodejs.org/job/node-test-pull-request/3377/

The test worries me though, we've had troubles relying on timeout duartions in tests too much. Perhaps there is a way to reduce the reliance on timeout durations.

erinishimoticha · 2016-07-21T20:35:22Z

@Fishrock123 Agree, I don't like relying the timeout duration one bit. I consulted with @julianduque to see if there was a better way. Do you have any suggestions? If refedLists was accessible we could check its length, but it's not. We can't rely on the 2nd timer's callback being called because it doesn't get called even when it's not canceled properly.

misterdjules · 2016-07-21T20:38:45Z

lib/timers.js

@@ -211,9 +211,9 @@ function listOnTimeout() {
  debug('%d list empty', msecs);
  assert(L.isEmpty(list));
  this.close();
-  if (list._unrefed === true) {
+  if (list._unrefed === true && list === unrefedLists[msecs]) {


Do you mind adding a block comment that describes why these checks are needed? Maybe something very similar to the block comment included with the newly added test would be a good fit.

The goal would be to make it clear to anyone reading the code (including those not familiar with it) why these extra checks are needed.

addaleax · 2016-07-21T21:00:09Z

test/parallel/test-timers-same-timeout-wrong-list-deleted.js

+    // erroneously deleted. If we are able to cancel the timer successfully,
+    // the bug is fixed.
+    clearTimeout(handle2);
+  }, 1), 10);


The , 1 argument to common.mustCall is optional and can be left out (which I would recommend here)

misterdjules · 2016-07-21T21:05:54Z

@erinspice One of the ways to not rely on timeout duration would be to check the output of process._getActiveHandles() from a "doubly nested" setImmediate, and make sure there's no timers list instance with a msecs property with a value === TIMEOUT in that output.

Something along the lines of:

const handle1 = setTimeout(common.mustCall(function() {
  // Cause the old TIMEOUT list to be deleted
  clearTimeout(handle1);

  // Cause a new list with the same key (TIMEOUT) to be created for this timer
  const handle2 = setTimeout(function() {
    common.fail('Inner callback is not called');
  }, TIMEOUT);

  setTimeout(common.mustCall(function() {
    // Attempt to cancel the second timer. Fix for this bug will keep the
    // newer timer from being dereferenced by keeping its list from being
    // erroneously deleted. If we are able to cancel the timer successfully,
    // the bug is fixed.
    clearTimeout(handle2);
    setImmediate(function() {
        setImmediate(function () {
          var activeHandles = process._getActiveHandles();
          checkNoTimersListWithTimeoutOf(TIMEOUT); // this would need to be implemented in this test
        })
    );
  }, 1), 10);

  // When this callback completes, `listOnTimeout` should now look at the
  // correct list and refrain from removing the new TIMEOUT list which
  // contains the reference to the newer timer.
}, 1), TIMEOUT);

The code above is just an example of what could be done, if you explore this solution feel free to change it :)

The problem with that approach is that it relies heavily on the current implementation of the libuv event loop. More specifically, it relies on the fact that the underlying libuv handle that was created for the timers list for timers with delays of TIMEOUT msecs is not active anymore after the immediate queue was processed twice.

It also relies on process._getActiveHandles(), which is not a supported API.

erinishimoticha · 2016-07-21T21:19:07Z

@misterdjules Thanks, that appears to work when running the test singly. Now running the entire suite. Will process._getActiveHandles return handles from other tests?

misterdjules · 2016-07-21T21:22:40Z

@erinspice

Will process._getActiveHandles return handles from other tests?

It should not, handles are per process.

misterdjules · 2016-07-21T21:24:27Z

After thinking a bit more about the problems mentioned in the testing approach suggested in my recent comment, it seems that the problem with that approach is that if _getActiveHandles starts not including timer handles that are active in its output, then there's the possibility of a false negative. Relying on the way immediate and timer handles' callbacks are called should be fine, since at worse it would make the test fail when the bug would still be fixed, which is OK.

Maybe combining the two methods, that is keeping the assert on durationOfTest < TIMEOUT * 2 and adding the method mentioned above is the best test we can come up with for now.

erinishimoticha · 2016-07-21T21:42:32Z

@misterdjules Sounds good.

misterdjules · 2016-07-21T21:44:16Z

@erinspice This PR is looking great so far. The comments in the test were very useful in order to understand how it was testing the change, and very well written. The commit message is also very clear. This is very good work with a great attention to details, and it is very much appreciated. Thank you!

erinishimoticha · 2016-07-21T21:46:43Z

My pleasure! I think I have addressed all the feedback so far.

misterdjules · 2016-07-21T22:07:59Z

test/parallel/test-timers-same-timeout-wrong-list-deleted.js

+    // the bug is fixed.
+    clearTimeout(handle2);
+    setImmediate(function() {
+      setImmediate(function() {


This could be nitpicking, but I think this callback should also be wrapped in a common.mustCall call. This would mean that the outer setImmediate callback would also need to be wrapped in a common.mustCall call, or that this inner setImmediate callback's definition be moved at the top level of the file and wrapped in a common.mustCall call there, and then used here.

misterdjules · 2016-07-21T22:09:45Z

I think this is looking good to me once the latest nit is addressed, but I'd like to wait for others, especially @Fishrock123, to weigh in too before giving a formal approval. Thank you once again!

Trott · 2016-07-21T23:19:25Z

If a different approach to testing is not found, one thing that might be helpful in mitigating any timer race condition stuff in the current incarnation is common.platformTimeout(). It's a hack we use to increase timeouts in situations where we expect the process to be slower (because of the type of hardware it is or because it's a debug build).

Another thing to do might be to have a collaborator run the stress test on Jenkins to make sure the test is not flaky. I'd recommend running the test 9999 times on a Windows machine and a SmartOS machine. If you want to be thorough, maybe one or two other things.

Lastly, there is a separate stress test for Raspberry Pi devices. I'd run the test 999 times on that.

I'm happy to kick off the stress tests if we think this is more-or-less in its probable final form.

misterdjules · 2016-07-22T00:27:38Z

If a different approach to testing is not found, one thing that might be helpful in mitigating any timer race condition stuff in the current incarnation is common.platformTimeout(). It's a hack we use to increase timeouts in situations where we expect the process to be slower (because of the type of hardware it is or because it's a debug build).

Just to make sure we're on the same page, there are (among others) two distinct problems with this test:

One that using common.platformTimeout solves, which is using timeout durations that are large enough so that, with this change, any platform on which tests are run can call the associated callback not too late after the delay that is specified. More specifically, on platforms that call timers' callbacks at a time that is >= timer delay * 2, using common.platformTimeout() to define the value of TIMEOUT would help avoid false positives.
Another that common.platformTimeout doesn't solve which is that having the process exit before a delay of 2 * TIMEOUT (even if TIMEOUT === common.platformTimeout(someDelay)) doesn't necessarily mean that the timer that should have been cleared and should have not held the libuv loop open was actually cleared (e.g the second timer could be still active and fire earlier due to another independent problem).

The use of nested immediates are an attempt at solving 2), but I think your suggestion for using common.platformTimeout in order to define a delay for TIMEOUT is great, thank you!

@erinspice Would you mind updating this PR by changing the value of TIMEOUT so that it's set to common.platformTimeout(100)?

We could get rid of the timing-based assertion if process._getActiveHandles was a supported API, and if the relative order with which immediates and timers are processed would be well defined, but I don't think this is currently the case for both.

In other words, keeping the timing-based test in combination with using nested immediates seems like a solution that could give false positives, but no false negatives if we consider that the current tests suite ensures that timers fire after delay ms elapsed, but never earlier.

Your suggestion about running stress tests should give us a better idea about the likeliness of false positives, but I expect that using common.platformTimeout to define TIMEOUT will make things better.

There is definitely room for improvement, and we might be able to come up with more robust mechanisms, both in the test added by that PR and in the rest of the timers tests. However I believe that what after @erinspice updates this PR to use common.platformTimeout(100) to define TIMEOUT, this PR will already be very solid, and it'll give me enough confidence in the changes that are being made.

Another thing to do might be to have a collaborator run the stress test on Jenkins to make sure the test is not flaky. I'd recommend running the test 9999 times on a Windows machine and a SmartOS machine. If you want to be thorough, maybe one or two other things.

Lastly, there is a separate stress test for Raspberry Pi devices. I'd run the test 999 times on that.

I think these are great suggestions.

I'm happy to kick off the stress tests if we think this is more-or-less in its probable final form.

After @erinspice makes the change to assigning the value common.platformTimeout(100) to TIMEOUT, I think this PR will be very close to its final form, but feel free to wait for @Fishrock123's feedback if running stress tests is costly.

misterdjules · 2016-07-22T00:47:28Z

test/parallel/test-timers-same-timeout-wrong-list-deleted.js

+    setImmediate(common.mustCall(function() {
+      setImmediate(common.mustCall(function() {
+        const activeHandles = process._getActiveHandles();
+        activeHandles.forEach(common.mustCall(function(handle) {


I think it's OK to leave the forEach callback as is, that is not wrapped by common.mustCall, unless we're somehow not confident that Array.forEach will call it activeHandles.length time.

However we should probably assert that activeHandles.length > 0, and that there's one instance of Timer in there (in addition to the assert already present).

I added the second argument to common.mustCall here because it was asserting a single call with no argument. There is no "called at least once" assertion. We could remove the mustCall completely from this loop.

The assertion that there is still one Timer remaining at this point fails, though, presumably because of the two calls to setImmediate? There are zero in the list of active handles.

This code passes:

setImmediate(common.mustCall(function() { setImmediate(common.mustCall(function() { const activeHandles = process._getActiveHandles(); const activeTimers = activeHandles.filter(function (handle) { return handle instanceof Timer; }); // Make sure our clearTimeout succeeded. One timer finished and // the other was canceled, so none should be active. assert.equal(activeTimers.length, 0, 'No Timers remain.'); })); }));

@misterdjules pushed.

@erinspice

The assertion that there is still one Timer remaining at this point fails, though, presumably because of the two calls to setImmediate? There are zero in the list of active handles.

Absolutely, my mistake, and sorry for the confusion. It's expected that no timers are left in the list of active handles at this point, so the latest changes are good.

Basically the goal is to have some check that Timer instances are actually added to the list of active handles at some point, otherwise checking for their absence doesn't tell us much. We know now that these Timer instances are added to the list of active handles, because we've been working on that test and we checked that ourselves, but future changes might change that and the test might pass just because Timer instances are never added to the list of active handles anymore.

What do you think of adding a check after the 10 ms timer is added that makes sure that there are at least two instances of Timer in the active handles list, one with a delay of TIMEOUT and the other with a delay of 10ms?

@misterdjules Done.

misterdjules · 2016-07-22T00:52:10Z

In other words, keeping the timing-based test in combination with using nested immediates seems like a solution that could give false positives, but no false negatives

Actually, in its current state we could still have false negatives, because there's no check that ensures that the activeHandles list contains all the Timer instances that we expect (that is, 1 instance with a delay of 10ms, in addition to checking that no timer instance with a delay of TIMEOUT is present.

I added a comment inline to mention that. @erinspice Do you mind updating this PR with that? And then I'll try to stop adding work to this PR :)

My apologies for the confusion.

misterdjules · 2016-07-27T00:32:54Z

@erinspice Thank you for the updates! Changes LGTM.

New CI run here: https://ci.nodejs.org/job/node-test-pull-request/3426/.

As per @Trott's suggestion:

Windows stress test running at https://ci.nodejs.org/job/node-stress-single-test/831/.
SmartOS stress test running at https://ci.nodejs.org/job/node-stress-single-test/830/.
Raspberry devices stress test running at https://ci.nodejs.org/view/All/job/node-stress-single-test-pi1-fanned/8/.

If the tests above do not identify any significant problem, we'll still wait a couple days so that someone else can also approve the change. After that delay, of if someone else also approves the changes, we'll be able to merge the changes in master.

We're getting closer, thank you @erinspice for your patience!

Trott · 2016-07-27T03:41:45Z

Only failure on CI is a Windows failure for parallel/test-net-reconnect-error.

https://ci.nodejs.org/job/node-test-binary-windows/3047/RUN_SUBSET=1,VS_VERSION=vs2013,label=win2008r2/console

not ok 165 parallel/test-net-reconnect-error
# TIMEOUT
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT error: ECONNREFUSED
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
# CLIENT disconnect
  ---
  duration_ms: 60.146

This test does not have a history of false positives, at least as far as I am aware. On the other hand, I wouldn't expect it to be affected by this change.

Here's another CI: https://ci.nodejs.org/job/node-test-pull-request/3428/

Stress test on parallel/test-net-reconnect-error using current master: https://ci.nodejs.org/job/node-stress-single-test/832/

Stress test on parallel/test-net-reconnect-error using this PR's code: ~~https://ci.nodejs.org/job/node-stress-single-test/833/~~ https://ci.nodejs.org/job/node-stress-single-test/834/

misterdjules · 2016-07-27T04:20:07Z

@nodejs/collaborators Does anyone have some time to review this? I would appreciate if another pair of eyes could take a look and formally give feedback. Thank you!

Trott · 2016-07-27T04:33:25Z

Very interesting: On master, parallel/test-net-reconnect-error routinely takes over 50 seconds, and the test harness times out after 60 seconds. Between that and the green CI re-run, I'd say we're in the clear on that test unless the stress test for the PR is significantly worse than the stress test for master. (So far, they're about the same.)

The test takes 50 seconds on some of the project's Windows CI infrastructure. Reducing the test repetitions from 50 to 20 trims that to about 20 seconds. Tests will timeout at 60 seconds, so this helps keep the test reliable. (There was a timeout on CI today when testing an unrelated code change.) Refs: nodejs#7827 (comment)

misterdjules · 2016-07-28T15:22:19Z

@nodejs/collaborators Last call to be able to review this, otherwise I'll land it probably later today, maybe tomorrow morning pacific time at the latest.

Thanks!

Trott · 2016-07-28T18:12:43Z

LGTM.

Is it worth running the timers benchmarks in benchmark/timers to compare this against current master?

misterdjules · 2016-07-28T19:12:14Z

@Trott I do not expect these changes to affect performance significantly, but it surely doesn't hurt to run the benchmarks, if only to confirm that theory.

misterdjules · 2016-07-28T22:18:45Z

@Trott

After running the following command:

./node benchmark/compare.js --old /tmp/node-master --new /tmp/node-7827 timers | tee /tmp/timers-compare-master-7827.csv

with /tmp/node-7827 being a binary built from the current tip of the nodejs/node's master branch with the changes from this PR applied, and /tmp/node-master being a binary built from the current tip of the nodejs/node's master branch, I got the following results after letting the benchmarks for ~3 hours:

➜  node git:(master) cat /tmp/timers-compare-master-7827.csv | Rscript benchmark/compare.R
                                                    improvement significant    p.value
 timers/immediate.js type="breadth" thousands=2000       7.75 %           * 0.02174587
 timers/immediate.js type="breadth1" thousands=2000      9.25 %             0.13107464
 timers/immediate.js type="breadth4" thousands=2000     10.28 %           * 0.01983081
 timers/immediate.js type="clear" thousands=2000        -6.70 %             0.12853650
 timers/immediate.js type="depth" thousands=2000         2.62 %             0.14894266
 timers/immediate.js type="depth1" thousands=2000        2.35 %             0.10804656
 timers/timers.js type="breadth" thousands=500          -2.81 %             0.10234362
 timers/timers.js type="depth" thousands=500            -0.40 %             0.63538975
➜  node git:(master)

These results seem to indicate that the only significant impact that we can say these changes possibly have with a significant level of confidence are performance improvements in setImmediate. However, these changes should not have an impact on setImmediate, so I would think that either the confidence level in these results is not high enough (hence the relatively high p-value, and only one star present), or there's something I'm missing.

Regardless, the benchmarks seem to not reject the null-hypothesis for these changes (p-values for the timers benchmarks are well above 0.05, and thus have no stars associated to them). Thus I'd think these changes are safe performance-wise.

The test takes 50 seconds on some of the project's Windows CI infrastructure. Reducing the test repetitions from 50 to 20 trims that to about 20 seconds. Tests will timeout at 60 seconds, so this helps keep the test reliable. (There was a timeout on CI today when testing an unrelated code change.) Refs: nodejs#7827 (comment) PR-URL: nodejs#7886 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: JungMinu - Minwoo Jung <jmwsoft@gmail.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

jasnell · 2016-07-29T15:49:35Z

LGTM

misterdjules · 2016-07-29T16:52:39Z

Landed in 7d75338. Thank you @erinspice, @Fishrock123, @Trott and @jasnell!

For nested timers with the same timeout, we can get into a situation where we have recreated a timer list immediately before we need to clean up an old timer list with the same key. Fix: make sure the list to be deleted is the same instance as the list whose reference was used to determine that a cleanup is necessary. If it's not the same instance, a new list with the same key has been created, and it should not be deleted. Fixes: #7722 PR-URL: #7827 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Julien Gilli <jgilli@fastmail.fm>

The test takes 50 seconds on some of the project's Windows CI infrastructure. Reducing the test repetitions from 50 to 20 trims that to about 20 seconds. Tests will timeout at 60 seconds, so this helps keep the test reliable. (There was a timeout on CI today when testing an unrelated code change.) Refs: #7827 (comment) PR-URL: #7886 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: JungMinu - Minwoo Jung <jmwsoft@gmail.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

For nested timers with the same timeout, we can get into a situation where we have recreated a timer list immediately before we need to clean up an old timer list with the same key. Fix: make sure the list to be deleted is the same instance as the list whose reference was used to determine that a cleanup is necessary. If it's not the same instance, a new list with the same key has been created, and it should not be deleted. Fixes: #7722 PR-URL: #7827 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Julien Gilli <jgilli@fastmail.fm>

MylesBorins · 2016-10-10T22:20:20Z

@Fishrock123 is this dependent on your timers re-write?

The test takes 50 seconds on some of the project's Windows CI infrastructure. Reducing the test repetitions from 50 to 20 trims that to about 20 seconds. Tests will timeout at 60 seconds, so this helps keep the test reliable. (There was a timeout on CI today when testing an unrelated code change.) Refs: #7827 (comment) PR-URL: #7886 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: JungMinu - Minwoo Jung <jmwsoft@gmail.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Jul 21, 2016

Fishrock123 reviewed Jul 21, 2016
View reviewed changes

misterdjules reviewed Jul 21, 2016
View reviewed changes

erinishimoticha force-pushed the erinspice/wrong-timer-list-deleted branch from 2d9fb7a to 1cbc063 Compare July 21, 2016 20:56

addaleax reviewed Jul 21, 2016
View reviewed changes

erinishimoticha force-pushed the erinspice/wrong-timer-list-deleted branch from 1cbc063 to 5cfd98d Compare July 21, 2016 21:44

misterdjules reviewed Jul 21, 2016
View reviewed changes

erinishimoticha force-pushed the erinspice/wrong-timer-list-deleted branch from 5cfd98d to 22241d4 Compare July 21, 2016 22:20

misterdjules reviewed Jul 22, 2016
View reviewed changes

erinishimoticha force-pushed the erinspice/wrong-timer-list-deleted branch 3 times, most recently from ae7640f to b54d9ab Compare July 26, 2016 22:21

erinishimoticha force-pushed the erinspice/wrong-timer-list-deleted branch from b54d9ab to 818a33f Compare July 26, 2016 22:24

Trott mentioned this pull request Jul 27, 2016

test: speed up test-net-reconnect-error #7886

Closed

2 tasks

misterdjules closed this Jul 29, 2016

misterdjules added lts-watch-v4.x and removed v6.x labels Jul 29, 2016

cjihrig mentioned this pull request Aug 8, 2016

v6.4.0 Release Proposal #8020

Closed

addaleax mentioned this pull request Aug 9, 2016

stream: avoid caching prepend check #8018

Closed

3 tasks

cjihrig mentioned this pull request Aug 11, 2016

v6.4.0 Proposal #8070

Merged

timers: fix cleanup of nested same-timeout timers #7827

timers: fix cleanup of nested same-timeout timers #7827

Conversation

erinishimoticha commented Jul 21, 2016

Checklist

Affected core subsystem(s)

Description of change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misterdjules commented Jul 21, 2016

Fishrock123 commented Jul 21, 2016

erinishimoticha commented Jul 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misterdjules commented Jul 21, 2016 • edited by thefourtheye Loading

erinishimoticha commented Jul 21, 2016

misterdjules commented Jul 21, 2016

misterdjules commented Jul 21, 2016 • edited Loading

erinishimoticha commented Jul 21, 2016

misterdjules commented Jul 21, 2016

erinishimoticha commented Jul 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misterdjules commented Jul 21, 2016

Trott commented Jul 21, 2016 • edited Loading

misterdjules commented Jul 22, 2016

Choose a reason for hiding this comment

erinishimoticha Jul 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misterdjules Jul 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misterdjules commented Jul 22, 2016

misterdjules commented Jul 27, 2016

Trott commented Jul 27, 2016 • edited Loading

misterdjules commented Jul 27, 2016

Trott commented Jul 27, 2016 • edited Loading

misterdjules commented Jul 28, 2016

Trott commented Jul 28, 2016

misterdjules commented Jul 28, 2016

misterdjules commented Jul 28, 2016 • edited Loading

jasnell commented Jul 29, 2016

misterdjules commented Jul 29, 2016

MylesBorins commented Oct 10, 2016

erinishimoticha commented Jul 21, 2016 •

edited

Loading

misterdjules commented Jul 21, 2016 •

edited by thefourtheye

Loading

misterdjules commented Jul 21, 2016 •

edited

Loading

erinishimoticha commented Jul 21, 2016 •

edited

Loading

Trott commented Jul 21, 2016 •

edited

Loading

erinishimoticha Jul 22, 2016 •

edited

Loading

misterdjules Jul 26, 2016 •

edited

Loading

Trott commented Jul 27, 2016 •

edited

Loading

Trott commented Jul 27, 2016 •

edited

Loading

misterdjules commented Jul 28, 2016 •

edited

Loading