win, pipe: avoid synchronous pipe deadlocks #1273

bzoz · 2017-03-23T17:15:02Z

Add a thread that will interrupt uv_pipe_zero_readdile_thread_proc every half second. This allows other processes to access the pipe without deadlocking. The "interrupt" is done by a call to CancelSynchronousIo, similar to how uv__pipe_getname pauses the read (see joyent/libuv#1313)

On Windows a call to uv_pipe_open will deadlock if another thread or process has ReadFile call active on that pipe.

This can happen when a process spawns Node with STDIN redirected to pipe. Libuv will call ReadFile in a separate thread (win/pipe.c:971). If this Node app spawns another child Node process with inherited stdio handles, this child process will hang. This is the reason behind nodejs/node#10836.

uv_pipe_open hangs on a call to NtQueryInformationFile (win/pipe.c:1931). This can be solved by using NtQueryObject. Unfortunately, this does not solve the issue - a later call to uv_set_pipe_handle (win/pipe.c:245) uses SetNamedPipeHandleState, GetNamedPipeHandleState and NtQueryInformationFile (with different parameters).

This is related to joyent/libuv#1313. But since the ReadFile is used in another process than uv_open_pipe we cannot synchronize them.

Ref: joyent/libuv#1313 (similar issue in libuv)
Ref: nodejs/node#10836 (issue in node fixed by this)

bzoz · 2017-04-03T14:12:08Z

@saghul could you take a look?

vtjnash · 2017-04-20T20:25:45Z

src/win/pipe.c

    uv_mutex_unlock(m);
  }
 restart_readfile:
+  if (hThreadForInterrupt) {
+    QueueUserWorkItem(uv__pipe_zero_read_interrupter,
+                      hThreadForInterrupt,


It's undefined what Win32 will do with this handle after this function returns (I expect it should reuse it for the next IO call). Provision needs to be made to ensure that the work item scheduled here doesn't try to access this handle asynchronously. This is what the mutex locks in uv__pipe_pause_read are required for.

bzoz · 2017-04-26T16:20:50Z

I've simplified the logic there. Now the "interrupter-thread" is created once and it calls CancelSynchronousIo in a loop. This thread will exit when uv_pipe_zero_readfile_thread_proc exits (WaitForSingleObject returns something different than WAIT_TIMEOUT)

@bnoordhuis @cjihrig could you review this?

@vtjnash I'm sorry, I don't really understand what is the issue here. The duplicated thread handle should be perfectly safe to be used in another thread without any synchronization. The mutex in uv__pipe_pause_read looks more like a way to signal uv_pipe_zero_readfile_thread_proc to resume the read.

cjihrig

A couple of comments. Mostly taking your word for it. I'd like at least one other person to review.

cjihrig · 2017-04-26T16:59:16Z

test/test-pipe-open-read-pipe.c

+  /* Give uv_run some time to start */
+  uv_sleep(250);
+  /* Try to access the pipe again, in different loop */
+  uv_loop_t test_loop;


Can you move this to the top of the function.

cjihrig · 2017-04-26T17:00:01Z

test/test-pipe-open-read-pipe.c

+}
+
+void pipe_read_thread_proc(void* arg) {
+  uv_pipe_t* pipe = arg;


Can you separate the assignment.

cjihrig · 2017-04-26T17:01:40Z

src/win/pipe.c

 static DWORD WINAPI uv_pipe_zero_readfile_thread_proc(void* parameter) {
  int result;
  DWORD bytes;
  uv_read_t* req = (uv_read_t*) parameter;
  uv_pipe_t* handle = (uv_pipe_t*) req->data;
  uv_loop_t* loop = handle->loop;
  HANDLE hThread = NULL;
+  HANDLE hThreadForInterrupt = NULL;


I think the assignment should be separate, but clearly the surrounding code isn't doing that.

vtjnash · 2017-04-26T17:18:33Z

The duplicated thread handle should be perfectly safe to be used in another thread without any synchronization

Why do you assume that? I would be surprised if the thread-pool was guaranteed to create a new thread for every work item submitted – that would be a lot more expensive than the mutex I'm suggesting may be necessary to synchronize the shared state between the two threads.

This can happen when a process spawns Node with STDIN redirected to pipe. Libuv will call ReadFile in a separate thread (win/pipe.c:971). If this Node app spawns another child Node process with inherited stdio handles, this child process will hang. This is the reason behind nodejs/node#10836.

I don't understand why this causes node to hang, since it doesn't look like your example code is reading from STDIN. Is node starting an active read on the STDIN pipe even without an .on("data") handler?

bzoz · 2017-04-26T21:15:26Z

In MSDN DuplicateHandle example it is used in similar way as here - a handle to object (in that case it is a mutex) is duplicated and passed on to a worker thread. Worker thread then uses the duplicated handle without any synchronization. Here we do exactly the same thing. Handle from GetCurrentThread() is duplicated into hThreadForInterrupt and passed on to thread-pool job. Worker thread uses this handle exclusively, it is not passed to any other thread or function.

Original node issue with repro using 3 files is here: nodejs/node#10836. My test case boils it down to the uv_pipe_open call on an anonymous pipe that is already read by another thread.

What happens in node is the following:

node process A spawns child node process B with {stdio: 'pipe'} option
node process B starts reading from stdin - because stdio is a pipe this creates uv_pipe_zero_readfile_thread_proc which calls ReadFile
node process B spawns child node process C with the default {stdio: inherit}
node process C access process.stdin - this will call uv_pipe_open and in turn NtQueryInformationFile on a pipe read by B and deadlock

vtjnash · 2017-04-26T22:02:50Z

Yes, that example is fine because it doesn't use the thread pool and explicitly uses the thread object itself as a mutex. This is valid, but very slow and resource intensive.

What happens in node is the following:

I was looking at that, but I don't see why node process B is reading from stdin. I don't see anywhere in the middle.js code that is starting a read on stdin?

bzoz · 2017-04-27T09:26:56Z

Sorry @vtjnash , I'm lost. What are your concerns exactly? I don't see anything particularly wrong with this code, am I missing something?

As for the read: it seems that node starts reading the pipe and then immediately pause it, as described here: nodejs/node#5776 (this was later reverted). In any case the reason for deadlock can be easily verified by attaching debugger to node processes.

bzoz · 2017-04-27T11:16:38Z

@cjihrig I've corrected the code.

I've also made the duplicated thread handle not inheritable.

vtjnash · 2017-04-27T14:39:54Z

The uv_pipe_zero_readfile_thread_proc function is called in a thread-pool (QueueUserWorkItem), so it does not own the thread that it is running on. It is undefined behavior to have any copy of this handle live when the uv_pipe_zero_readfile_thread_proc function returns.

As for the read

This sounds like a bug in nodejs. It shouldn't be initializing and attempting a read when it doesn't want the data (since that wastes resources – and causes the hang observed here).

bzoz · 2017-04-27T15:11:04Z

Ah, I see now. I was missing the part that uv_pipe_zero_readfile_thread_proc is not a "normal" thread. Thanks for pointing it out, you are right, I'll fix that.

As for node - the linked PR tried to fix that and it caused other problems. OTOH, a process could be reading from stdin and create child processes, and we would still get this bug.

vtjnash · 2017-04-27T15:36:01Z

OTOH, a process could be reading from stdin and create child processes, and we would still get this bug

Not necessarily – that could also be considered a bug in the javascript app that is trying to simultaneously consume stdin from two places. (instead of incurring the overhead here of starting an extra thread and waking up both threads twice every second).

bzoz · 2017-05-08T11:42:13Z

I've updated the PR code, PTAL.

vtjnash · 2017-05-08T15:33:13Z

src/win/pipe.c

+  uv__pipe_interrupter_param_t* param;
+
+  param = (uv__pipe_interrupter_param_t*) uv__malloc(
+    sizeof(uv__pipe_interrupter_param_t));


this should be able to use alloca – the application needs to guarantee that this pointer is never accessed after the function returns, to avoid race conditions, anyways.

vtjnash · 2017-05-08T15:34:22Z

src/win/pipe.c

+  for (;;) {
+    r = WaitForSingleObject(interrupterParam->completedEvent,
+                            PIPE_ZERO_READ_INTERRUPT_INTERVAL);
+    if (r == WAIT_TIMEOUT) {


this is still race-y. getting a wait-timeout previous to here doesn't guarantee that the thread handle is still valid after here.

vtjnash · 2017-05-08T15:38:31Z

src/win/pipe.c

@@ -46,6 +46,9 @@ struct uv__ipc_queue_item_s {
 /* A zero-size buffer for use by uv_pipe_read */
 static char uv_zero_[] = "";

+/* To prevent deadlocks we will interrupt synchronous pipe read every 500 ms */
+#define PIPE_ZERO_READ_INTERRUPT_INTERVAL 500


This seems very frequent - especially for something that will likely be caused by stdio, having timers that need to get woken up at a 500ms period seems like it could have a measurable impact on battery life. For non-nodejs consumers of this library (i.e. programs that don't share the nodejs bug, like my use case of JuliaLang), there should also be a way to disable this code.

bzoz · 2017-05-11T16:35:26Z

@vtjnash I've updated the code. I've added a mutex for synchronization, bumped interval to 2.5s and made entire thing opt-in by a call to uv_pipe_enable_interrupter.

bzoz · 2017-06-06T09:21:17Z

ping?

bzoz · 2017-06-19T08:58:44Z

pong?

bzoz · 2017-07-07T15:48:03Z

ping?

bnoordhuis · 2017-07-07T16:16:20Z

Sorry Bartosz, I missed that you asked for a review. I've only looked at this briefly but it seems like a rather inelegant solution. What would happen if uv_pipe_open() duplicates the handle before using it?

bzoz · 2017-07-07T16:31:04Z

I guess this would affect both handles. I'll double check.

bzoz · 2017-07-13T12:50:14Z

@bnoordhuis each uv_pipe_t instance needs a separate call to uv_pipe_enable_interrupter.

I know this is not an elegant solution, and as such per @vtjnash suggestion it is on-demand. But this is the only way I know of solving this, which is a real-world issue.

bnoordhuis

I don't think it's a very aesthetically pleasing solution but if you fix the outstanding issues, I'll sign off on it. Thanks.

bnoordhuis · 2017-08-08T14:22:11Z

src/win/pipe.c

+typedef struct {
+  volatile HANDLE thread;
+  HANDLE threadMutex;
+  HANDLE completedEvent;


Minor stylistic issue: snake_case, not camelCase.

bnoordhuis · 2017-08-08T14:23:07Z

src/win/pipe.c

+  uv__readfile_interrupter_t* interrupter;
+  int r;
+
+  interrupter = (uv__readfile_interrupter_t*) param;


Superfluous cast?

bnoordhuis · 2017-08-08T14:23:42Z

src/win/pipe.c

+      }
+      ReleaseMutex(interrupter->threadMutex);
+    } else {
+      /* ReadFile thread has terminated - cleanup the handles and exit */


Typo: clean up

bnoordhuis · 2017-08-08T14:24:07Z

src/win/pipe.c

+  uv__readfile_interrupter_t* interrupter;
+
+  interrupter = (uv__readfile_interrupter_t*) uv__malloc(
+    sizeof(uv__readfile_interrupter_t));


sizeof(*interrupter). Also, superfluous cast.

bnoordhuis · 2017-08-08T14:24:25Z

src/win/pipe.c

+}
+
+static uv__readfile_interrupter_t* uv__start_readfile_interrupter() {
+  HANDLE threadHandle;


Style: snake_case, not camelCase.

bnoordhuis · 2017-08-08T14:24:47Z

src/win/pipe.c

+    goto failed_event;
+  }
+  if (!QueueUserWorkItem(uv__readfile_interrupter_thread, interrupter,
+    WT_EXECUTEINLONGTHREAD)) {


Can you line up the argument?

bnoordhuis · 2017-08-08T14:25:57Z

src/win/pipe.c

+  interrupter = (uv__readfile_interrupter_t*) uv__malloc(
+    sizeof(uv__readfile_interrupter_t));
+  if (interrupter == NULL) {
+    goto failed_malloc;


I'd just return NULL here. No need to jump, nothing to clean up.

bnoordhuis · 2017-08-08T14:26:12Z

src/win/pipe.c

+    WT_EXECUTEINLONGTHREAD)) {
+    goto failed_queue;
+  }
+  goto done;


Just return interrupter?

bnoordhuis · 2017-08-08T14:27:16Z

test/test-pipe-open-read-pipe.c

+  HANDLE stdin_read_pipe, stdin_write_pipe;
+  SECURITY_ATTRIBUTES sa_attr;
+
+  sa_attr.nLength = sizeof(SECURITY_ATTRIBUTES);


sizeof(sa_attr)?

saghul · 2017-08-08T21:14:52Z

I took a quick look... and it's nasty 😞 Not your fault @bzoz!

Is there a reason why we don't do this always for sync pipes? I mean, who wouldn't want to avoid deadlocks? Specially given users don't know if a pipe is synchronous or not, right?

vtjnash · 2017-08-08T21:40:28Z

The operation is a no-op for asynchronous pipes (they don't have the same issue in the kernel). I requested that it not be always enable for synchronous pipes since this is only needed as a workaround for a nodejs bug. Other frontends (for example, my use case is Julia), don't need the extra overhead of spawning and synchronizing yet another thread.

saghul · 2017-08-08T22:01:40Z

Can you clarify why it's not needed? If it's a kernel thing, why isn't Julia running into the same problem?

vtjnash · 2017-08-08T22:12:33Z

Julia's standard library doesn't execute a read on a pipe if there's no active reader at the language / user level. That ensures it's only an issue in edge cases (two readers in separate processes trying to consume tokens off of the same pipe).

bnoordhuis

LGTM modulo nits. Should the interval be configurable?

bnoordhuis · 2017-08-09T08:35:57Z

src/win/pipe.c

+}
+
+static uv__readfile_interrupter_t* uv__start_readfile_interrupter() {
+  HANDLE thread_handle;


() -> (void) - () is a function taking any number of arguments.

bnoordhuis · 2017-08-09T08:37:34Z

src/win/pipe.c

@@ -985,6 +1079,8 @@ static DWORD WINAPI uv_pipe_zero_readfile_thread_proc(void* parameter) {
          uv_mutex_lock(m);
          handle->pipe.conn.readfile_thread = hThread;
          uv_mutex_unlock(m);
+          /* Give some time for other processes to wake before restarting. */
+          Sleep(1);


Is this necessary? What happens if you remove it?

Unfortunately - yes. Without it the test won't work. Interrupter thread will be stuck at CancelSynchronousIo, zero-read at ReadFile, and the test thread at NtQueryInformationFile. For whatever reason zero-read thread seems to ignore the CancelSynchronousIo call, unless a Sleep (or a printf, or a attached debugger) is present.

This should be conditional on interrupter.

bnoordhuis · 2017-08-09T08:38:03Z

src/win/pipe.c

@@ -1010,6 +1106,9 @@ static DWORD WINAPI uv_pipe_zero_readfile_thread_proc(void* parameter) {
  }

  POST_COMPLETION_FOR_REQ(loop, req);
+  if (interrupter) {


if (interrupter != NULL) {

bnoordhuis · 2017-08-09T08:39:45Z

test/test-pipe-open-read-pipe.c

+  ASSERT(r == 0);
+
+  /* Give uv_run some time to start */
+  uv_sleep(250);


I hope this isn't going to be flaky...

bzoz · 2017-08-09T15:25:53Z

@bnoordhuis updated, PTAL

vtjnash · 2017-08-09T18:21:16Z

src/win/pipe.c

+        SwitchToThread();
+      }
+      ReleaseMutex(interrupter->thread_mutex);
+    } else {


if (WAIT_OBJECT_0)

bzoz · 2017-08-10T14:01:01Z

@vtjnash updated, PTAL

bzoz · 2017-08-17T19:51:02Z

If no one objects, I will land this tomorrow.

saghul · 2017-08-18T07:32:00Z

I still don't like that one function that is super-specific. Also pretty much Node only AFAWCT. Can we wake this behavior a compile-time define? We can always turn it into a function if it's needed later, but I'd really want to avoid adding this weird to use API.

Add a thread that will interrupt uv_pipe_zero_readdile_thread_proc every two and half second. This allows other processes to access the pipe without deadlocking Ref: nodejs/node#10836

vtjnash · 2017-08-18T15:47:27Z

Can we wake this behavior a compile-time define?

I thought so far that libuv has been avoiding compile-time defines? This one seems like it would be especially inconvenient for system packagers (if there are any on Windows?), as it forces them to choose between bad latency (read monitoring is only active 70% of the time with this PR) for all applications or nodejs/node#10836.

vtjnash · 2017-08-21T19:15:28Z

See also #95 (comment), which this PR implements.

bzoz · 2017-08-22T08:29:38Z

Using WaitForSingleObject was the first thing I tried, since I basically want to add a timeout to the ReadFile. I was not able to make it work, it looks like one cannot synchronize on pipes (see MSDN).

@vtjnash I've added a slight break which happens every 2.5s. Does this really end in the read being active only 70% of the time?

I think that this API is suboptimal. Basically each synchronous pipe will be affected by this bug and can cause a deadlock. I think that all synchronous pipes read should have interrupter enabled. I will drop uv_pipe_enable_interrupter and make it opt-in through an option to uv_loop_configure.

vtjnash

I've added a slight break

Ah, no, right. I always forget that the units of Sleep are milliseconds not seconds.

Basically each synchronous pipe will be affected by this bug and can cause a deadlock

Only pipes that are inherited or passed to another program can cause deadlock.

vtjnash · 2017-08-22T13:28:37Z

src/win/pipe.c

+
+  if (!QueueUserWorkItem(uv__readfile_interrupter_thread,
+                         interrupter,
+                         WT_EXECUTEINLONGTHREAD))


use RegisterWaitForSingleObject to avoid needing to allocate a new thread

MSDN says I can't wait on a pipe.

I could use CreateTimerQueueTimer to use the threadpool to wake interrupter every 2.5s, but I think that would complicate things.

Sadly true. I spent several hours yesterday trying to work around the NT kernel behavior (and failing), and wishing that it was open source.

But that's not why you're allocating this work item. This thread call can be replaced with a RegisterWaitForSingleObject callback.

On what handle should I wait?

interrupter->completed_event

bzoz · 2017-08-23T10:24:26Z

So, adding an option to uv_loop_configure won't work, because there is no flags or anything similar in uv_loop_t on Windows ([1], [2]).

So again - while this is not a nice API, and implementation is basically a hack, I don't see any other option.

BTW, this has been open for 5 months now 🎂

vtjnash · 2017-08-23T17:09:11Z

I don't see any other option.

Got any twitter followers on the NT Kernel team? :) I'm still hoping that someday that they'll fix this bug.

Is it a competition? I've got #1498 with 5 years and counting 😛. You are far more active around here than me though.

saghul · 2017-08-24T07:26:54Z

I thought so far that libuv has been avoiding compile-time defines?

Special problems call for special measures.

BTW, this has been open for 5 months now 🎂

It has been broken for a lot longer though 🤷‍♂️

bzoz · 2017-08-24T12:09:46Z

Since I cannot add this to uv_loop_configure, maybe we should add a global configuration (eg. uv_configure)? Besides opting-in for this, it could maybe be used to opt-out from user-mode symlinks (#1466) ?

saghul · 2017-08-25T07:45:42Z

Besides opting-in for this, it could maybe be used to opt-out from user-mode symlinks (#1466) ?

Why would anyone ever want that?

saghul · 2017-08-25T07:47:07Z

My stance on this, unless there is a new approach which we haven't seen yet is to do it compile time.

bzoz · 2017-08-29T23:29:12Z

The user-mode symlinks thing was only an example.

I'm reluctant to make this compile time. Mostly I think because there is no such mechanism already in place. IMHO a call to uv_configure (or uv_loop_configure with NULL as loop parameter) would be nicer and easier to maintain than a new build feature.

BTW, @vtjnash are you sure it should not be default in libuv? Even though in this specific situation is caused by the way Node handles stdio, Julia app could receive a pipe that is already being read as one of its stdio handles. Trying to initialize such pipe with uv_pipe_open will deadlock any runtime.

vtjnash · 2017-08-30T02:14:50Z

I don't think that protecting applications against deadlock caused by other misbehaving applications is really in the purview of libuv. For more context, this was also discussed on the original PR (joyent/libuv#1377 (comment)). I'll admit that it's unsatisfying that the deadlock could be avoided if Microsoft fixed their kernel. However it should be rare to encounter situations that it would occur (an application that spawns nodejs after spawning ReadFile in a thread pool on the same NamedPipe it passes to nodejs to use as stdin – note that this situation also creates an race condition of two processes trying to consume user keystroke input).

bzoz · 2017-09-06T12:58:10Z

As compile time option: #1525

vtjnash · 2017-09-08T18:35:43Z

Trying to initialize such pipe with uv_pipe_open will deadlock any runtime

This PR does not address this case. This PR can only help with working around nodejs bugs, there is nothing that can be done in libuv that would work around bugs in other applications.

bzoz · 2018-02-14T12:21:17Z

The issue in Node.js was solved, this makes this PR obsolete. Closing.

vtjnash reviewed Apr 20, 2017

View reviewed changes

bzoz force-pushed the bartek-fix-stdio-hange branch from 29c2844 to 0fa84df Compare April 26, 2017 16:07

cjihrig reviewed Apr 26, 2017

View reviewed changes

bzoz force-pushed the bartek-fix-stdio-hange branch from 0fa84df to dd6c4db Compare April 27, 2017 11:15

bzoz force-pushed the bartek-fix-stdio-hange branch from dd6c4db to 590ba6d Compare May 8, 2017 11:41

vtjnash reviewed May 8, 2017

View reviewed changes

bzoz force-pushed the bartek-fix-stdio-hange branch from 590ba6d to c94b219 Compare May 11, 2017 16:33

bzoz requested review from bnoordhuis, saghul and santigimeno June 12, 2017 08:27

Trott mentioned this pull request Jul 15, 2017

Accessing process.stdin hangs on Windows nodejs/node#10836

Closed

bnoordhuis requested changes Aug 8, 2017

View reviewed changes

bnoordhuis approved these changes Aug 9, 2017

View reviewed changes

vtjnash reviewed Aug 9, 2017

View reviewed changes

src/win/pipe.c Outdated

SwitchToThread();

}

ReleaseMutex(interrupter->thread_mutex);

} else {

Copy link

Member

vtjnash Aug 9, 2017

Choose a reason for hiding this comment

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

if (WAIT_OBJECT_0)

bnoordhuis approved these changes Aug 10, 2017

View reviewed changes

win, pipe: avoid synchronous pipe deadlocks

c4d887e

Add a thread that will interrupt uv_pipe_zero_readdile_thread_proc every two and half second. This allows other processes to access the pipe without deadlocking Ref: nodejs/node#10836

bzoz force-pushed the bartek-fix-stdio-hange branch from 84ce471 to c4d887e Compare August 18, 2017 08:53

vtjnash reviewed Aug 22, 2017

View reviewed changes

bzoz mentioned this pull request Sep 6, 2017

win, pipe: avoid synchronous pipe deadlocks - as compile time option #1525

Closed

bzoz closed this Feb 14, 2018

win, pipe: avoid synchronous pipe deadlocks #1273

win, pipe: avoid synchronous pipe deadlocks #1273

Conversation

bzoz commented Mar 23, 2017

bzoz commented Apr 3, 2017

Choose a reason for hiding this comment

bzoz commented Apr 26, 2017

cjihrig left a comment

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

vtjnash commented Apr 26, 2017

bzoz commented Apr 26, 2017

vtjnash commented Apr 26, 2017

bzoz commented Apr 27, 2017

bzoz commented Apr 27, 2017

vtjnash commented Apr 27, 2017

bzoz commented Apr 27, 2017

vtjnash commented Apr 27, 2017

bzoz commented May 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzoz commented May 11, 2017

bzoz commented Jun 6, 2017

bzoz commented Jun 19, 2017

bzoz commented Jul 7, 2017

bnoordhuis commented Jul 7, 2017

bzoz commented Jul 7, 2017

bzoz commented Jul 13, 2017

bnoordhuis left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saghul commented Aug 8, 2017

vtjnash commented Aug 8, 2017

saghul commented Aug 8, 2017

vtjnash commented Aug 8, 2017

bnoordhuis left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzoz commented Aug 9, 2017

Choose a reason for hiding this comment

bzoz commented Aug 10, 2017

bzoz commented Aug 17, 2017

saghul commented Aug 18, 2017

vtjnash commented Aug 18, 2017

vtjnash commented Aug 21, 2017

bzoz commented Aug 22, 2017

vtjnash left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzoz commented Aug 23, 2017

vtjnash commented Aug 23, 2017

saghul commented Aug 24, 2017

bzoz commented Aug 24, 2017

saghul commented Aug 25, 2017

saghul commented Aug 25, 2017

bzoz commented Aug 29, 2017

vtjnash commented Aug 30, 2017

bzoz commented Sep 6, 2017

vtjnash commented Sep 8, 2017 • edited Loading

bzoz commented Feb 14, 2018

vtjnash commented Sep 8, 2017 •

edited

Loading