src: clean up resources on Environment teardown #19377

addaleax · 2018-03-15T20:47:35Z

Opening this in reference to #19365, porting commits over from Ayo (that also serve as Worker support preparation):

src: add environment cleanup hooks

This adds pairs of methods to the Environment class and to public APIs
which can add and remove cleanup handlers.

Unlike AtExit, this API targets addon developers rather than
embedders, giving them (and Node’s internals) the ability to register
per-Environment cleanup work.

We may want to replace AtExit with this API at some point.

(This commit is semver-minor.)
src: make CleanupHandles() tear down handles/reqs

Previously, handles would not be closed when the current Environment
stopped, which is acceptable in a single-Environment-per-process
situation, but would otherwise create memory and file descriptor
leaks.

Also, introduce a generic way to close handles via the
Environment::CloseHandle() function, which automatically keeps
track of whether a close callback has been called yet or not.
src: unify ReqWrap libuv calling

This allows easier tracking of whether there are active ReqWraps.
src: keep track of open requests

Workers cannot shut down while requests are open, so keep a counter
that is increased whenever libuv requests are made and decreased
whenever their callback is called.

This also applies to other embedders, who may want to shut down
an Environment instance early.
src: use cleanup hooks to tear down BaseObjects

Clean up after BaseObject instances when the Environment
is being shut down. This takes care of closing non-libuv resources
like zlib instances, which do not require asynchronous shutdown.
src: add can_call_into_js flag

This prevents calls back into JS from the shutdown phase.
tools: remove --quiet from run-valgrind.py

This should no longer be an issue, now that we clean up
resources when exiting.

New since the PR was opened:

Libuv cherry pick for win, pipe: stop read for overlapped pipe libuv/libuv#1822 – blocked on that for now
src: always call ReadStop() before Close()

For libuv-backed streams, always explicitly stop reading before
closing the handle.

(Note: I’m not sure whether this is necessary and will kick off CI both with and without the commit.)

lib: defer pausing stdin to the next tick

This is done to match the stream implementation, which also
only actually stops reading in the next tick after the 'pause'
event is emitted.
src: remove NodeCategorySet destructor

This currently crashes during environment cleanup because
the object would be torn down while there are enabled categories.

I’m not sure about the exact semantics here, but since the
object cannot be garbage collected at this point anyway
because it’s Persistent handle is strong, removing the
destructor at least doesn’t make anything worse than it is
right now (i.e. the destructor would never have been called
before anyway).
src: store fd for libuv streams on Windows

On Windows, we can't just look up a FD for libuv streams and
return it in GetFD().
However, we do sometimes construct streams from their FDs;
in those cases, it should be okay to store the value on a class field.
process: create stdin with manualStart: true

Otherwise Node.js will try to read data from the handle.
This causes issues when Node.js is already reading from the
same handle, but a different associated stream
(e.g. a possible IPC channel).

Almost all commits have been reviewed in their original form by @Qard (and one by @TimothyGu and @aqrln), and partly have been talked through with them. Many, many thanks for that! 💙

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines
make tests pass on Windows
take care of handles without HandleWraps

/cc @joyeecheung @jasnell @yhwang @gireeshpunathil

addaleax · 2018-03-15T20:52:29Z

CI: https://ci.nodejs.org/job/node-test-commit/16939/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1334/
Benchmark CI for fs.stat() (because we should at least benchmark something): https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/141/

addaleax · 2018-03-15T23:13:01Z

Sadly, the parallel/test-child-process-std{in,out}-ipc failures seem real and don’t provide any helpful information whatsoever. If anybody from @nodejs/platform-windows could take a look, that would be great, otherwise I’ll try to dig into it on the weekend.

joyeecheung · 2018-03-16T02:44:35Z

src/env.cc

+
+  while (!cleanup_hooks_.empty()) {
+    // Copy into a vector, since we can't sort an unordered_set in-place.
+    std::vector<CleanupHookCallback> callbacks(


nit: maybe just swap the two vectors?

The other one is not a vector :)

joyeecheung

Very nice work! Left some comments but I think they are ignorable, will take a deeper look over the weekend.

joyeecheung · 2018-03-16T02:47:06Z

src/env.h

@@ -854,6 +872,32 @@ class Environment {
  void RunAndClearNativeImmediates();
  static void CheckImmediate(uv_check_t* handle);

+  struct CleanupHookCallback {
+    void (*fun_)(void*);


nit: I think we usually go with fn_ or cb_ for these kinds of members?

joyeecheung · 2018-03-16T02:53:58Z

src/node_file.cc

+// Convert a C++ lambda function to a raw, C-style function.
+// This is helpful, because ReqWrap::Dispatch() does not recognize lambda
+// functions, and thus does not wrap them properly.
+typedef void(*uv_fs_callback_t)(uv_fs_t*);


I believe this only works on non-capturing lambdas? Also simply initializing a uv_fs_callback_t with the lambda inline when they are going to be passed probably works as well?

wrap->Dispatch(..., uv_fs_callback_t{[](uv_fs_t*) { // lambda code here }})

I believe this only works on non-capturing lambdas?

We wouldn’t be able to pass others to libuv anyway. But yes, I think it only works with them.

Also simply initializing a uv_fs_callback_t with the lambda inline when they are going to be passed probably works as well?

Heh. Yes. 😄

joyeecheung · 2018-03-16T02:59:11Z

src/env-inl.h

@@ -527,6 +554,14 @@ void Environment::SetUnrefImmediate(native_immediate_callback cb,
  CreateImmediate(cb, data, obj, false);
 }

+inline bool Environment::can_call_into_js() const {
+  return can_call_into_js_;


I think technically, we cannot call into JS whenever there is a pending exception? Is is possible for someone to bump into a pending exception while this returns true?

Is is possible for someone to bump into a pending exception while this returns true?

I would expect that, yes. But the code should be laid out to handle such a situation anyway, right?

The function isn’t meant to give an exhausting result that tells code whether it’s okay to call into JS or not… it’s more of an “allowed to” than “able to”. Maybe we should rename it to may_call_into_js()? What do you think?

@addaleax In my understanding may_call_into_js() is probably not too different from can_call_into_js(), maybe add a comment there making it clear that this returning true does not guarantee that one can call into the JS without being careful about pending exceptions? Or, since we only set it to false...maybe cannot_call_into_js()?

Or, if we don't want to be too board about the meaning of this...maybe just something like is_in_tear_down()?

maybe add a comment there making it clear that this returning true does not guarantee that one can call into the JS without being careful about pending exceptions?

Sure, I’m adding a comment. :)

Or, if we don't want to be too board about the meaning of this...maybe just something like is_in_tear_down()?

I think I’d prefer to name it according to how its consumers use it. I don’t know why we would, but I wouldn’t consider it impossible that at some point we may want to use this flag for other things too.

gireeshpunathil · 2018-03-16T05:10:34Z

@addaleax - thanks for this amazing work!

src: add environment cleanup hooks:

I assume Environment::RunCleanup() is the teardown API, should we have some docs around that?

src: make CleanupHandles() tear down handles/reqs

I guess this covers all the handles that were created through the JS code. How about the handles created by the embedders directly through uv_*_init means, not through the Node's opaque *Wrap means?

src: add can_call_into_js flag

Once the tear down is initiated, how is this (calling into JS land) possible? is this specific to worker case? Should we have this here?

One of the test case I have in my mind is (basically derived from my goal through #19365 ) is to be able to do something like this:

int main() {
  while (1) { // or some condition
    void *params = read(config);
    node::Start(params);
 }
}
static void acb(uv_async_t* handle) {
// tear down on demand, on some condition
  Environment::RunCleanup();
}

And validate that at every loop back edges, the run time state is same as the baseline - in terms of memory, threads, fds, ... I will come up with something and test this code.

addaleax · 2018-03-16T10:09:37Z

I assume Environment::RunCleanup() is the teardown API, should we have some docs around that?

Hm – @joyeecheung I think we should call that function as part of FreeEnvironment() now … does that sound right to you?

I guess this covers all the handles that were created through the JS code. How about the handles created by the embedders directly through uv_*_init means, not through the Node's opaque *Wrap means?

Hm, yes, there might be few ones that would be left and should be included here.

I think it’s only the uv_async_init + uv_timer_init calls from cares_wrap.cc that are relevant here, though.

Once the tear down is initiated, how is this (calling into JS land) possible?

It’s totally possible, we just don’t want to allow it because a) it’s inconsistent with the current behaviour of dropping everything when exiting and b) it could schedule arbitrary amounts of work by creating new handles and requests.

(We could make an exception of some sort for stdio, though. This PR might be a good starting point for the “flush on exit” problem we’ve been having for years…)

is this specific to worker case? Should we have this here?

It’s not specific to the worker case, no.

And validate that at every loop back edges, the run time state is same as the baseline - in terms of memory, threads, fds, ... I will come up with something and test this code.

Fwiw, for memory that is more or less covered by testing with valgrind, which now passes for most tests. (Yay! 🎉)

bnoordhuis

Left some suggestions but by and large LGTM.

bnoordhuis · 2018-03-16T08:02:13Z

doc/api/n-api.md

+current Node.js environment exits.
+
+A function can safely be specified multiple times with different
+`arg` values. In that case, it will be called multiple times as well.


Probably worth specifying in which order the hooks run.

bnoordhuis · 2018-03-16T08:10:56Z

src/cares_wrap.cc

-           [](uv_handle_t* handle) {
-    delete reinterpret_cast<uv_timer_t*>(handle);
-  });
+  env()->CloseHandle(timer_handle_, [](uv_timer_t* handle){ delete handle; });


Space before {.

bnoordhuis · 2018-03-16T08:12:01Z

src/cares_wrap.cc

-  req_wrap->Dispatched();
+  int err = req_wrap->Dispatch(uv_getnameinfo,
+                               AfterGetNameInfo,
+                               (struct sockaddr*)&addr,


Since you're here: reinterpret_cast

bnoordhuis · 2018-03-16T08:34:59Z

src/env.h

+
+    // We keep track of the insertion order for these objects, so that we can
+    // call the callbacks in reverse order when we are cleaning up.
+    int64_t insertion_order_counter_;


uint64_t? Same on line 899.

bnoordhuis · 2018-03-16T08:43:23Z

src/node_contextify.cc

@@ -1076,6 +1076,8 @@ class ContextifyScript : public BaseObject {
                          const bool break_on_sigint,
                          const FunctionCallbackInfo<Value>& args,
                          TryCatch* try_catch) {
+    if (!env->can_call_into_js())
+      return false;


All the return false exits in this function are from pending exceptions except this one. Probably harmless because:

no callers use the return value, and

all callers return to JS land afterwards

bnoordhuis · 2018-03-16T08:46:22Z

src/node_crypto.cc

@@ -5124,6 +5127,7 @@ void PBKDF2(const FunctionCallbackInfo<Value>& args) {
  if (args[5]->IsFunction()) {
    obj->Set(env->context(), env->ondone_string(), args[5]).FromJust();

+    env->IncreaseWaitingRequestCounter();


Maybe do this from the constructor and destructor? I assume it's harmless in the sync case?

Right now it’s always done directly before the request is started and when the callback is called. I think that makes things easier to keep track off.

bnoordhuis · 2018-03-16T09:02:07Z

src/req_wrap-inl.h

+
+/* Below is dark template magic designed to invoke libuv functions that
+ * initialize uv_req_t instances in a unified fashion, to allow easier
+ * tracking of active/inactive requests. */


C++ comments, please.

bnoordhuis · 2018-03-16T09:04:31Z

src/req_wrap-inl.h

+// Detect `int uv_foo(uv_loop_t* loop, uv_req_t* request, ...);`.
+template <typename ReqT, typename... Args>
+struct CallLibuvFunction<ReqT, int(*)(uv_loop_t*, ReqT*, Args...)> {
+  typedef int(*T)(uv_loop_t*, ReqT*, Args...);


Might be easier to read as a using T = ... declaration (and F is arguably a better name for it.)

bnoordhuis · 2018-03-16T10:13:16Z

src/req_wrap-inl.h

+  if (err >= 0)
+    env()->IncreaseWaitingRequestCounter();
+  return err;
+}


Just thinking out aloud but maybe there's a way to make it more flexible / comprehensible / less prone to obscure compile errors? Quick sketch:

template <typename T> class LibuvCallback { public: using InitF = std::function<int(*)(T*, void (*)(T*, int))>; using DoneF = std::function<void(*)(std::unique_ptr<LibuvCallback>, int)>; T* req() { return &req_; } private: template <typename> friend int MakeLibuvCallback(Environment*, InitF, DoneF); std::unique_ptr<LibuvCallback> self_; Environment* const env_; DoneF done_; T req_; LibuvCallback(Environment* env, DoneF done) : env_(env), done_(done), req_() {} static void Callback(T* req, int status) { LibuvCallback* that = ContainerOf(&LibuvCallback::req_, req); that->env_->DecreaseWaitingRequestCounter(); that->done_(std::move(that->self_), status); } }; template <typename T> int MakeLibuvCallback( Environment* env, LibuvCallback<T>::InitF init, LibuvCallback<T>::DoneF done) { using C = LibuvCallback<T>; std::unique_ptr<C> that(new C(env, done)); if (int err = init(&that->req_, C::Callback)) return err; env->IncreaseWaitingRequestCounter(); that->self_ = std::move(that); return 0; }

And used like this:

{ Environment* env = /* ... */; const char* path = /* ... */; auto init_cb = [=] (uv_fs_t* req, uv_fs_cb cb) { return uv_fs_stat(env->event_loop(), req, path, cb); }; auto done_cb = [=] (std::unique_ptr<LibuvCallback<uv_fs_t>> that, int status) { // do something with that->req() // auto-deleted when |that| goes out of scope }; int err = MakeLibuvCallback(env, init_cb, done_cb); // ... }

The std::unique_ptr is to make it possible to extend the lifetime of the request beyond the done callback. It shouldn't do more heap allocations than currently, assuming the compiler is smart enough to unbox the init callback.

I like the approach, and I’ll think about whether we can implement it here, but one of the reasons for the current complexity is that libuv requests don’t always match a (uv_foo_t* request, int status) signature. Otherwise this would be a lot simpler. :)

jasnell · 2018-03-16T14:17:37Z

doc/api/n-api.md

@@ -890,6 +890,53 @@ If still valid, this API returns the `napi_value` representing the
 JavaScript Object associated with the `napi_ref`. Otherise, result
 will be NULL.

+### Cleanup on exit of the current Node.js instance
+
+While a Node.js typically releases all its resources when exiting, embedders


s/While a Node.js/While Node.js

jasnell · 2018-03-16T14:18:22Z

doc/api/n-api.md

+                                                     void* arg);
+```
+
+Un-registers `fun` as a function to be run with the `arg` parameter once the


s/Un-registers/Unregisters

jasnell · 2018-03-16T14:18:55Z

doc/api/n-api.md

+need to be exact matches.
+
+The function must have originally been registered
+with `napi_add_env_cleanup_hook`.


What happens if it's not?

Nothing, and we can’t really do anything about that, but it doesn’t make sense to call this function otherwise.

As long as it's a non-op in that case, ++

yhwang · 2018-03-16T18:57:11Z

doc/api/n-api.md

+Registers `fun` as a function to be run with the `arg` parameter once the
+current Node.js environment exits.
+
+A function can safely be specified multiple times with different


one question: if I call napi_add_env_cleanup_hook multiple times with the same function and arg, how can I just remove one of them when calling napi_remove_env_cleanup_hook?

Edit: I saw unorder_set::emplace() and erase(). In my case, it only add one callback into the cleanup hook.

@yhwang You can’t do it, right now it would crash with that – I’ve clarified this in the documentation.

The reason is that you basically never want that. If you pass the same arg + fun combination twice in a meaningful way, that means that you rely on global state; in that case, your code will probably not work anyway in the scenarios that this is meant to support.

(Another reason is the suspicion – not benchmarked – that a std::unordered_set is more performant than a std::unordered_multiset.)

If you pass the same arg + fun combination twice in a meaningful way, that means that you rely on global state

hmm, the arg could be a refcount object and decrease its counter in each call of the callback function :-p

I saw you said with different arg values now. Good catch!

If you pass the same arg + fun combination twice in a meaningful way, that means that you rely on global state

hmm, the arg could be a refcount object and decrease its counter in each call of the callback function :-p

Yeah, you could still get that kind of behaviour by creating pointers with an additional layer of indirection if you really want to and using those as unique keys. But like I said, probably not something one really wants to do :)

joyeecheung · 2018-03-16T19:13:17Z

Hm – @joyeecheung I think we should call that function as part of FreeEnvironment() now … does that sound right to you?

SGTM

addaleax · 2018-03-16T22:17:22Z

Updated with most feedback applied, still haven’t gotten around to looking into the Windows failures

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

yhwang · 2018-03-16T23:55:51Z

src/base_object-inl.h

@@ -39,6 +39,12 @@ inline BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> handle)
  // nullptr in case it's accessed by the user before construction is complete.
  if (handle->InternalFieldCount() > 0)
    handle->SetAlignedPointerInInternalField(0, nullptr);
+  env_->AddCleanupHook(DeleteMe, static_cast<void*>(this));


will DeleteMe() somehow conflict with delete data.GetParameter(); in BaseObject::WeakCallback()? should DeleteMe() call ClearWeak() ?
I didn't read through all of the code please forgive me if I ask stupid question. Right now seems two mechanisms for clean up, one is v8::PersistentBase< T >::SetWeak() which relies on V8 GC and the env cleanup hook you add now. could them be merged into one?

will DeleteMe() somehow conflict with delete data.GetParameter(); in BaseObject::WeakCallback()?

I don’t think so… when one of these mechanisms is called, the other gets disabled in the destructor. That should work, right?

Should DeleteMe() call ClearWeak() ?

The delete self; call destroys the persistent handle, so I don’t think calling ClearWeak() on that handle before it would make a difference?

I didn't read through all of the code please forgive me if I ask stupid question.

No no, please keep these questions (which are not stupid) coming :) It’s good to talk this through.

could them be merged into one?

Hm – how would we do this? There isn’t much code being added here, so I’m not sure how to merge those…

Also, there’s a third way to cleanup a BaseObject: To delete it explicitly somewhere else, like we do e.g. for HandleWraps. I agree that lifetime management in Node core is currently not the easiest thing to keep track off, though.

jasnell · 2018-03-17T00:16:11Z

Looking quickly at the windows failure... it's in a specific child process test. While I'm waiting for the windows build to actually complete, I'm making a somewhat completely random guess that there might be a missing DecreaseWaitingRequestCounter? Perhaps?

addaleax · 2018-03-17T00:35:46Z

@jasnell Yeah, that’s a possibility, but I’m not sure how that would fail in a platform-dependent manner.

It might be an instance of a libuv request not finishing, like libuv/libuv#1729 (if that’s a real bug) or a similar issue. But even that’s a long shot without doing any investigation. :)

Refs: #19377 PR-URL: #20585 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

This adds pairs of methods to the `Environment` class and to public APIs which can add and remove cleanup handlers. Unlike `AtExit`, this API targets addon developers rather than embedders, giving them (and Node\u2019s internals) the ability to register per-`Environment` cleanup work. We may want to replace `AtExit` with this API at some point. Many thanks for Stephen Belanger for reviewing the original version of this commit in the Ayo.js project. Refs: ayojs/ayo#82 PR-URL: nodejs#19377 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

This adds pairs of methods to the `Environment` class and to public APIs which can add and remove cleanup handlers. Unlike `AtExit`, this API targets addon developers rather than embedders, giving them (and Node\u2019s internals) the ability to register per-`Environment` cleanup work. We may want to replace `AtExit` with this API at some point. Many thanks for Stephen Belanger for reviewing the original version of this commit in the Ayo.js project. Refs: ayojs/ayo#82 Backport-PR-URL: #22435 PR-URL: #19377 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: nodejs#19377 PR-URL: nodejs#20585 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-PR-URL: #24103 Refs: #19377 PR-URL: #20585 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

This adds pairs of methods to the `Environment` class and to public APIs which can add and remove cleanup handlers. Unlike `AtExit`, this API targets addon developers rather than embedders, giving them (and Node’s internals) the ability to register per-`Environment` cleanup work. We may want to replace `AtExit` with this API at some point. Many thanks for Stephen Belanger for reviewing the original version of this commit in the Ayo.js project. Refs: ayojs/ayo#82 PR-URL: nodejs#19377 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 15, 2018

addaleax mentioned this pull request Mar 15, 2018

Ability to shutdown Node in-flight #19365

Closed

addaleax force-pushed the cleanup branch 2 times, most recently from 3a4d878 to c207e6e Compare March 16, 2018 00:09

joyeecheung reviewed Mar 16, 2018

View reviewed changes

addaleax added the wip Issues and PRs that are still a work in progress. label Mar 16, 2018

bnoordhuis approved these changes Mar 16, 2018

View reviewed changes

jasnell reviewed Mar 16, 2018

View reviewed changes

yhwang reviewed Mar 16, 2018

View reviewed changes

addaleax force-pushed the cleanup branch 3 times, most recently from d70f0e9 to 01f9041 Compare March 16, 2018 22:12

jasnell approved these changes Mar 16, 2018

View reviewed changes

yhwang reviewed Mar 16, 2018

View reviewed changes

tmpfs mentioned this pull request Jun 18, 2018

No output for mkcat <filename> mkdoc/mkcat#1

Open

MylesBorins pushed a commit that referenced this pull request Aug 7, 2018

deps: upgrade to libuv 1.20.3

e751460

Refs: #19377 PR-URL: #20585 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

gabrielschulhof mentioned this pull request Aug 21, 2018

[v8.x-backport] src: add environment cleanup hooks #22435

Closed

4 tasks

gabrielschulhof mentioned this pull request Aug 31, 2018

[v6.x backport] src: add environment cleanup hooks #22633

Closed

4 tasks

addaleax mentioned this pull request Sep 2, 2018

Worker (Multi) Threads nodejs/help#1473

Closed

MylesBorins mentioned this pull request Sep 6, 2018

v8.12.0 proposal #21593

Merged

MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Nov 5, 2018

deps: upgrade to libuv 1.20.3

49910ce

Refs: nodejs#19377 PR-URL: nodejs#20585 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Nov 11, 2018

deps: upgrade to libuv 1.20.3

c54f4bc

Backport-PR-URL: #24103 Refs: #19377 PR-URL: #20585 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

src: clean up resources on Environment teardown #19377

src: clean up resources on Environment teardown #19377

Conversation

addaleax commented Mar 15, 2018 • edited Loading

Checklist

addaleax commented Mar 15, 2018

addaleax commented Mar 15, 2018

joyeecheung Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung Mar 16, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gireeshpunathil commented Mar 16, 2018

addaleax commented Mar 16, 2018

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

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

yhwang Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

joyeecheung commented Mar 16, 2018

addaleax commented Mar 16, 2018

yhwang Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Mar 17, 2018

addaleax commented Mar 17, 2018

addaleax commented Mar 15, 2018 •

edited

Loading

joyeecheung Mar 16, 2018 •

edited

Loading

joyeecheung Mar 16, 2018 •

edited

Loading

yhwang Mar 16, 2018 •

edited

Loading

addaleax Mar 16, 2018 •

edited

Loading

yhwang Mar 16, 2018 •

edited

Loading