src: Node implementation of v8::Platform by matthewloring · Pull Request #14001 · nodejs/node

matthewloring · 2017-06-30T11:01:16Z

Node.js currently uses the V8 implementation of the DefaultPlatform
which schedules VM tasks on a V8 managed thread pool. Since the Node.js
event loop is not aware of these tasks, the Node.js process may exit
while there are outstanding VM tasks. This will become problematic once
asynchronous wasm compilation lands in V8.

This PR introduces a Node.js specific implementation of the v8::Platform
on top of libuv so that the event loop is aware of outstanding VM tasks.

Fixes: #12980

Checklist

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

Affected core subsystem(s)

src, tracing

matthewloring · 2017-06-30T11:01:47Z

/cc @jeisinger @nodejs/v8

matthewloring · 2017-06-30T11:04:50Z

v8/v8@3d8e87a was reverted upstream in V8 due to tsan failures in multi-isolate scenarios. It should be safe for use by Node.js but we can also swap out that commit once it re-lands in V8 before landing this PR.

(edit by @addaleax: turned the git commit ref into a v8/v8@… ref)

addaleax

Commented with a few style nits but I think that’s it.

CI: https://ci.nodejs.org/job/node-test-commit/10841/

addaleax · 2017-06-30T11:46:51Z

src/node_platform.cc

static_cast?

addaleax · 2017-06-30T11:46:55Z

src/node_platform.cc

static_cast?

addaleax · 2017-06-30T11:47:24Z

src/node_platform.cc

tiny style nit: uv_work_t* req (like below)

addaleax · 2017-06-30T11:55:49Z

src/node_platform.cc

static_cast here and in the one above as well. ;)

bnoordhuis · 2017-06-30T12:31:50Z

deps/v8/include/libplatform/libplatform.h

FYI, conflicts with #13985.

That has landed and this PR has been rebased.

bnoordhuis · 2017-06-30T12:37:09Z

src/node.cc

This needs to be updated to the new prototype.

bnoordhuis · 2017-06-30T12:39:50Z

src/node.cc

4 space indent.

@bnoordhuis Is there a guideline anywhere for C++ code indentation? I (and others possibly) am usually fairly uncertain where the 4-space rule should apply, and where 2-space.

I’m not sure we’ve written it done somewhere, but generally, use 4 spaces if it’s a statement continuation, 2 spaces if it’s indentation for a block/loop body/conditional statement body/etc.

I've updated to 4 spaces.

bnoordhuis · 2017-06-30T12:41:16Z

src/node.cc

These are never freed?

They are now freed in Dispose.

bnoordhuis · 2017-06-30T12:48:09Z

src/node_platform.cc

s/req/handle/?

Done throughout.

bnoordhuis · 2017-06-30T12:49:18Z

src/node_platform.cc

1e9 is a little easier to parse for humans. Can you punctuate the comment?

bnoordhuis · 2017-06-30T12:50:02Z

src/node_platform.h

Can you move these to the .cc file and use fully qualified names here? Rule of thumb: no using declarations in headers.

bnoordhuis · 2017-06-30T12:50:27Z

src/node_platform.h

Can you call this e.g. num_threads_?

TimothyGu · 2017-06-30T13:48:26Z

src/node_platform.cc

Maybe these internal functions could be made static or part of an anonymous namespace?

Good point – they might be best written as lambdas, for readability?

I found that with lambdas there were too many variables named handle. I've made them static for now. I can switch to lambdas if that is preferred.

TimothyGu · 2017-06-30T13:56:02Z

src/node_platform.h

Are node_mutex.h and queue actually used anywhere within the file?

matthewloring · 2017-06-30T14:11:20Z

Thanks for the comments! I'm currently looking into some intermittent crashes in GC brought on by this change. I'll address these comments once I figure that out.

mhdawson · 2017-07-06T15:17:28Z

src/node_platform.h

Should this just be Isolate* or should the others be v8::Isolate*

These should all be v8::Isolate.

mhdawson · 2017-07-06T15:20:51Z

src/node_platform.h

Would be nice if the API for NodePlatform did not depend on v8 specific types like v8::TracingController and v8::Isolate (below) but it is still a good start just be abstracting NodePlatform itself.

This was discussed briefly in #12980. This is not an attempt to define a general VM interface. The only goal is to provide a new implementation of v8::Platform as the default platform which is currently used by Node will may allow the process to exit during wasm module compilation.

jeisinger · 2017-07-21T07:03:43Z

src/node_platform.cc

ASAN complains about this line, it looks like uv_run() tries to delete this handle as well.

This needs to call uv_close() and not free the memory until the close callback is called.

Ok, does the same logic apply to the uv_work_t in AfterBackgroundTask?

bnoordhuis · 2017-07-21T14:07:16Z

src/node.cc

Style nit: four space indent.

bnoordhuis · 2017-07-21T14:07:40Z

src/node.cc

Can you set tracing_agent_ = nullptr;?

bnoordhuis · 2017-07-21T14:12:19Z

src/node_platform.cc

This needs to call uv_close() and not free the memory until the close callback is called.

bnoordhuis · 2017-07-21T14:13:34Z

src/node_platform.cc

Can you call new uv_work_t() here? Coverity tends to complain about heap-allocating structures without initializing them. Same a few lines below with the new uv_timer_t call.

bnoordhuis · 2017-07-21T14:14:00Z

src/node_platform.cc

Typo: guarantee

bnoordhuis · 2017-07-21T14:15:11Z

src/node_platform.h

Maybe make these const since they'll never change.

I've made the thread count const. uv_timer_init and uv_queue_work don't like const loops.

I mean uv_loop_t* const loop, i.e., non-reseatable pointer. Not a big thing though.

bnoordhuis · 2017-07-21T14:17:31Z

src/tracing/trace_event.cc

Since you're here: g_controller? We use underscore suffix convention really only for data members.

matthewloring · 2017-07-21T17:51:45Z

Trying the CI: https://ci.nodejs.org/job/node-test-pull-request/9287/

matthewloring · 2017-07-21T22:19:40Z

The CI looks mostly green. The repl timeout seems unrelated. The test-timers-unrefed-in-beforeexit failure is real. If V8 queues up background work during the execution of the beforeExit function, the event loop stays open waiting for that work to complete and then reruns the beforeExit callback again. This can cause the process to hang for sufficiently complicated beforeExit callbacks (I can reproduce this by allocating lots of buffers in beforeExit).

One option would be to drop all V8 background tasks queued during the execution of beforeExit. However, this could be problematic if the user expects to recover and continue execution. @jeisinger Is it possible for background tasks to be dropped or will this leave V8 in an inconsistent state?

jeisinger · 2017-07-24T09:34:30Z

some background tasks from the GC will count down semaphores while the main thread waits for the semaphore, so not running background tasks will result in a deadlock on the main thread.

matthewloring · 2017-07-25T01:36:39Z

If background tasks are allowed to be queue during the beforeExit callback, the process could repeatedly invoke beforeExit forever. If background tasks are buffered during the execution of beforeExit and flushed afterwards, the process could deadlock. I experimented with starting another thread at the beginning of beforeExit that could flush background tasks on a timer to make progress if a deadlock occurs. However this is sub-optimal as the process would still hang until the timer fires or beforeExit could still be executing when the timer fires. @jeisinger Would it be possible for the platform API to expose a task priority or some other indicator of whether a task should hold the process open? Longer term would it be possible to move away from blocking the main thread on background task completion?

matthewloring · 2017-08-01T23:16:10Z

Flushing all background tasks after beforeExit executes has appeased tests locally. New CI: https://ci.nodejs.org/job/node-test-pull-request/9442/.

addaleax

Just reaffirming that this still LGTM

matthewloring · 2017-08-04T16:13:26Z

For an update on this, I worked with the V8 team to stop the main thread from blocking on background task completion (reflected in the most recent 3 backports on the PR). A more desirable fix would be to let background tasks started inside beforeExit run and then block until all have completed before determining whether the event loop still has active handles/requests and the process should stay up. However, since background tasks are run using uv_queue_work, there will be active requests in the event loop even after blocking until all background tasks have completed the background portion of their work. The event loop must be run at least once to clear these requests (so that they won't be observed when uv_loop_alive is checked) but there seem to be a fair number of unit tests that depended on no io polling before the uv_loop_alive check. If there was a way to cancel these background tasks after their background work was completed to avoid having their after callbacks added to the event loop or a way to unref reqs as can be done with handles that would solve the problem. @bnoordhuis do you have any thoughts on a work around for this problem?

The current solution will only be problematic if V8 background tasks started inside beforeExit queue foreground tasks as those new foreground tasks will not keep the process up (as will be the case for WASM asynchronous compilation). However, no current V8 background tasks have that behavior to my knowledge so this should be safe for now.

I've started a fresh CI here: https://ci.nodejs.org/job/node-test-pull-request/9486/

matthewloring · 2017-08-04T17:41:26Z

It looks like there is one remaining related failure which is on windows: https://ci.nodejs.org/job/node-test-binary-windows/10290/RUN_SUBSET=1,VS_VERSION=vs2017,label=win2016/tapResults/. I can try to get my hands on a windows machine but if anyone on @nodejs/platform-windows can reproduce this or has thoughts on why libuv is unhappy I'd appreciate the help.

matthewloring · 2017-08-06T21:04:29Z

After a few more fixes, it looks like inspector/test-off-with-session-then-on is failing on windows intermittently. @eugeneo Do you have any thoughts on these failures? (example CI: https://ci.nodejs.org/job/node-test-commit/11587/).

tniessen

src/* LGTM

Original commit message: [heap] Move SweeperTask to CancelableTask This mitigates the problem of blocking on the main thread when the platform is unable to execute background tasks in a timely manner. Bug: v8:6655 Change-Id: Icdaae744ee73146b86b9a28c8035138746721971 Reviewed-on: https://chromium-review.googlesource.com/595467 Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#47036} PR-URL: nodejs#14001 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Original commit message: Make CancelableTask ids unique They were only limited to 32 bit when using the internal Hashmap. Since this has changed alreay some time ago, we can switch to 64 bit ids and check that we never overflow. Bug: Change-Id: Ia6c6d02d6b5e555c6941185a79427dc4aa2a1d62 Reviewed-on: https://chromium-review.googlesource.com/598229 Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#47085} PR-URL: nodejs#14001 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Original commit message: [heap] Move UnmapFreeMemoryTask to CancelableTask This mitigates the problem of blocking on the main thread when the platform is unable to execute background tasks in a timely manner. Bug: v8:6671 Change-Id: I741d4b7594e8d62721dad32cbfb19551ffacd0c3 Reviewed-on: https://chromium-review.googlesource.com/599528 Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#47126} PR-URL: nodejs#14001 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Fishrock123 · 2017-09-15T21:24:11Z

src/node.cc

-        if (uv_run(env.event_loop(), UV_RUN_NOWAIT) != 0)
-          more = true;
-      }
+      uv_run(env.event_loop(), UV_RUN_DEFAULT);


Is everyone here sure this doesn't break something edge-casey with beforeExit and order of operations?

I'm trying to dig into it but have pretty much run out of time this week.

It seems to me that in some case(s) before, uv callbacks may have been called twice before the next beforeExit, which could alter things at the very least like an addon that uses a uv_check_t handle?

MylesBorins · 2017-09-20T15:24:00Z

I'm assuming that this is not applicable to the 6.x branch. Please let me know if I am mistaken

/cc @nodejs/v8 @nodejs/tsc

Original commit message: [heap] Move SweeperTask to CancelableTask This mitigates the problem of blocking on the main thread when the platform is unable to execute background tasks in a timely manner. Bug: v8:6655 Change-Id: Icdaae744ee73146b86b9a28c8035138746721971 Reviewed-on: https://chromium-review.googlesource.com/595467 Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#47036} PR-URL: nodejs#14001 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Original commit message: Make CancelableTask ids unique They were only limited to 32 bit when using the internal Hashmap. Since this has changed alreay some time ago, we can switch to 64 bit ids and check that we never overflow. Bug: Change-Id: Ia6c6d02d6b5e555c6941185a79427dc4aa2a1d62 Reviewed-on: https://chromium-review.googlesource.com/598229 Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#47085} PR-URL: nodejs#14001 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Original commit message: [heap] Move UnmapFreeMemoryTask to CancelableTask This mitigates the problem of blocking on the main thread when the platform is unable to execute background tasks in a timely manner. Bug: v8:6671 Change-Id: I741d4b7594e8d62721dad32cbfb19551ffacd0c3 Reviewed-on: https://chromium-review.googlesource.com/599528 Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#47126} PR-URL: nodejs#14001 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

V8 modified the platform API to accept a tracing controller at platform creation time that is required to be present for the lifetime of the platform if tracing will every be enabled. This will simplify the implementation of a v8::Platform subclass for node. PR-URL: nodejs/node#14001 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Original commit message: [heap] Move SweeperTask to CancelableTask This mitigates the problem of blocking on the main thread when the platform is unable to execute background tasks in a timely manner. Bug: v8:6655 Change-Id: Icdaae744ee73146b86b9a28c8035138746721971 Reviewed-on: https://chromium-review.googlesource.com/595467 Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{#47036} Backport-PR-URL: #15393 PR-URL: #14001 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Original commit message: Make CancelableTask ids unique They were only limited to 32 bit when using the internal Hashmap. Since this has changed alreay some time ago, we can switch to 64 bit ids and check that we never overflow. Bug: Change-Id: Ia6c6d02d6b5e555c6941185a79427dc4aa2a1d62 Reviewed-on: https://chromium-review.googlesource.com/598229 Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{#47085} Backport-PR-URL: #15393 PR-URL: #14001 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Original commit message: [heap] Move UnmapFreeMemoryTask to CancelableTask This mitigates the problem of blocking on the main thread when the platform is unable to execute background tasks in a timely manner. Bug: v8:6671 Change-Id: I741d4b7594e8d62721dad32cbfb19551ffacd0c3 Reviewed-on: https://chromium-review.googlesource.com/599528 Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{#47126} Backport-PR-URL: #15393 PR-URL: #14001 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Original commit message: [heap] Move SweeperTask to CancelableTask This mitigates the problem of blocking on the main thread when the platform is unable to execute background tasks in a timely manner. Bug: v8:6655 Change-Id: Icdaae744ee73146b86b9a28c8035138746721971 Reviewed-on: https://chromium-review.googlesource.com/595467 Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{#47036} Backport-PR-URL: #15393 PR-URL: #14001 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Original commit message: Make CancelableTask ids unique They were only limited to 32 bit when using the internal Hashmap. Since this has changed alreay some time ago, we can switch to 64 bit ids and check that we never overflow. Bug: Change-Id: Ia6c6d02d6b5e555c6941185a79427dc4aa2a1d62 Reviewed-on: https://chromium-review.googlesource.com/598229 Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{#47085} Backport-PR-URL: #15393 PR-URL: #14001 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Original commit message: [heap] Move UnmapFreeMemoryTask to CancelableTask This mitigates the problem of blocking on the main thread when the platform is unable to execute background tasks in a timely manner. Bug: v8:6671 Change-Id: I741d4b7594e8d62721dad32cbfb19551ffacd0c3 Reviewed-on: https://chromium-review.googlesource.com/599528 Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{#47126} Backport-PR-URL: #15393 PR-URL: #14001 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Original commit message: [heap] Move SweeperTask to CancelableTask This mitigates the problem of blocking on the main thread when the platform is unable to execute background tasks in a timely manner. Bug: v8:6655 Change-Id: Icdaae744ee73146b86b9a28c8035138746721971 Reviewed-on: https://chromium-review.googlesource.com/595467 Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{#47036} Backport-PR-URL: #15393 PR-URL: #14001 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Original commit message: Make CancelableTask ids unique They were only limited to 32 bit when using the internal Hashmap. Since this has changed alreay some time ago, we can switch to 64 bit ids and check that we never overflow. Bug: Change-Id: Ia6c6d02d6b5e555c6941185a79427dc4aa2a1d62 Reviewed-on: https://chromium-review.googlesource.com/598229 Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{#47085} Backport-PR-URL: #15393 PR-URL: #14001 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Original commit message: [heap] Move UnmapFreeMemoryTask to CancelableTask This mitigates the problem of blocking on the main thread when the platform is unable to execute background tasks in a timely manner. Bug: v8:6671 Change-Id: I741d4b7594e8d62721dad32cbfb19551ffacd0c3 Reviewed-on: https://chromium-review.googlesource.com/599528 Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{#47126} Backport-PR-URL: #15393 PR-URL: #14001 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

addaleax approved these changes Jun 30, 2017

View reviewed changes

addaleax reviewed Jun 30, 2017

View reviewed changes

bnoordhuis requested changes Jun 30, 2017

View reviewed changes

TimothyGu reviewed Jun 30, 2017

View reviewed changes

mhdawson reviewed Jul 6, 2017

View reviewed changes

jeisinger mentioned this pull request Jul 19, 2017

let v8 use libuv thread pool #11855

Closed

jeisinger reviewed Jul 21, 2017

View reviewed changes

bnoordhuis requested changes Jul 21, 2017

View reviewed changes

bnoordhuis previously approved these changes Jul 21, 2017

View reviewed changes

matthewloring mentioned this pull request Aug 2, 2017

test/parallel/test-async-wrap-getasyncid.js crashing #14599

Closed

addaleax approved these changes Aug 4, 2017

View reviewed changes

tniessen approved these changes Aug 7, 2017

View reviewed changes

Fishrock123 reviewed Sep 15, 2017

View reviewed changes

MylesBorins added the dont-land-on-v6.x label Sep 20, 2017

MylesBorins mentioned this pull request Oct 3, 2017

v8.7.0 proposal #15762

Merged

justus-hildebrand mentioned this pull request Jan 29, 2018

Verify TickEventLoop code is correct hpicgs/node#27

Closed

davisjam mentioned this pull request Sep 3, 2018

WIP: Pluggable thread pool #22631

Closed

4 tasks

zbjornson mentioned this pull request Jan 30, 2019

fs.readFile is slower in v10 #25741

Closed

gengjiawen mentioned this pull request Mar 25, 2019

src: apply clang-tidy rule bugprone-incorrect-roundings #26885

Merged

2 tasks

addaleax added the v8 platform Issues and PRs related to Node's v8::Platform implementation. label Feb 18, 2020

Uh oh!

Conversation

matthewloring commented Jun 30, 2017

Checklist

Affected core subsystem(s)

Uh oh!

matthewloring commented Jun 30, 2017

Uh oh!

matthewloring commented Jun 30, 2017 • edited by addaleax Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

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!

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!

matthewloring commented Jun 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewloring commented Jun 30, 2017 •

edited by addaleax

Loading