Guide on "Don't block the event loop" #1478

davisjam · 2017-11-19T04:21:33Z

Towards #1471.

Not finished yet, hoping to get some initial feedback.

Tiriel

Haven't finished ready in entirety yet, but one general opinion yet: I think you should go for a more formal tone.
This sounds too much like a blog post to me, not like an "official" guide. That may not be the general feeling of everyone, but I for one would prefer a more neutral tone on a guide found on Node.js official website.

Coming soon: complete review, although I'm not as technical as some!

davisjam · 2017-11-19T19:27:36Z

Thanks for the comment on tone. I will try a more formal approach in my next revision.

Thoughts about the technical content?

Tiriel

As said, I'm not the most technical reviewer you could get, I'm largely a noob in Node.js and JS itself. I still hope my comments will help!

Tiriel · 2017-11-19T20:08:26Z

locale/en/docs/guides/dont-block-the-event-loop.md

+
+A safer alternative is the [node-re2](https://github.com/uhop/node-re2) project, which uses Google's blazing-fast [RE2](https://github.com/google/re2) regexp engine. But be warned, RE2 is not 100% compatible with Node's regexps, so check for regressions if you swap in the node-re2 module to handle your regexps. And really complicated regexps may not be supported by node-re2.
+
+As usual, if you're trying to match something "obvious", like a URL or a file path, use an npm module!


Quick question here, mostly for my own education:
In that case, wouldn't stringVar.indexOf(searchVar) !== -1 or stringVar.includes(searchVar) !== false be more efficient, and eliminating the need of another module?

I originally come from PHP, and replacing preg_match() with stripos($string, $search) !== false is mostly preferred for the same reasons, so I thought it would be the same in JS.

If you're just doing a strcmp or substr-style string match, then yes, you should just use indexOf. You don't need a regular expression for this, nor will a regexp actually be dangerous in this context.

But sometimes people need more matching power for more complicated expressions, and a sufficiently complicated regexp can lead to REDOS.

Okay, that's reassuring 😄
Maybe in that case you could specify this alternative, and that sufficiently complex regex are the real problem here? (I'm not quite sure what a sufficiently complete regex would be either; regex using escaped/reserved chars and/or logn regex?)

Is the new REDOS section clearer? Still needs work?

Tiriel · 2017-11-19T20:13:43Z

locale/en/docs/guides/dont-block-the-event-loop.md

+
+This gives all of the other pending clients a turn.
+It's pretty easy to accomplish this by sticking whatever state you're carrying around in a closure or an object.
+And you might want to add a TODO to revisit this section of your code later.


Some example might be good here. I think most newcomers/basic users won't necessarily have a clue about what you're talking here. The notion of 'immediate' in the event loop can be misleading and some people won't understand why it would partly solve the problem IMHO.

Good idea. I've been thinking about some examples.

Tiriel · 2017-11-19T20:14:07Z

locale/en/docs/guides/dont-block-the-event-loop.md

+And you might want to add a TODO to revisit this section of your code later.
+
+## Don't block the Worker Pool
+Work in progress.


Damn, that was the piece I was most eager to read 😄

Alrighty, first draft of this is up.

vsemozhetbyt · 2017-11-19T22:27:44Z

@davisjam It seems your local email in git settings does not match your GitHub one. Can you add a local one to GitHub email list so that your contributions to Node.js repositories can be associated with your GitHub account?

For nodejs#1471

davisjam · 2017-11-19T22:51:51Z

@vsemozhetbyt Thanks, always forget this.

davisjam · 2017-11-19T23:49:05Z

@Tiriel:

Is the language up to an appropriate level of formality, or should I work on the tone further?
Does the regexp section make more sense now?

I haven't revised anything after the REDOS section yet.

- REDOS - JSON DOS - "Complex calculations without blocking the Event Loop"

Tiriel · 2017-11-20T08:20:51Z

@davisjam Way better now IMHO! It's more precise, the examples add a lot, and the tone is better suited to the subject and target as I see it.

davisjam · 2017-11-20T18:13:03Z

The first draft of the full document is up. Comments appreciated.

ayaankazerouni · 2017-11-20T18:42:43Z

+1 Cool guide! I learned a lot, especially from the REDOS section.

davisjam · 2017-11-21T00:32:01Z

I'm not the most technical reviewer you could get, I'm largely a noob in Node.js and JS itself

@Tiriel When you have a moment, can you ping some appropriate folks for technical feedback? Your thoughts on style and correctness are of course also much appreciated.

vsemozhetbyt · 2017-11-21T01:00:58Z

Let's cc @nodejs/documentation for a start.

gibfahn · 2017-11-21T09:05:14Z

cc/ @sam-github if you're around, seems like something you'd be interested in.

sam-github

Its good to provide some information on this, particularly to provide implementation advice to users. It would be easy for someone to read this and think that network I/O occurs in the uv thread pool (which it doesn't), I think that aspect should be much more clear.

sam-github · 2017-11-21T21:25:15Z

locale/en/docs/guides/dont-block-the-event-loop.md

+
+## Should you read this guide?
+If you're writing a server using Node, keep reading.
+If you're writing command-line scripts with Node, this guide might still come in handy, but it isn't crucial.


cli scripts can do, for example, heavy DNS lookups at the same time as file I/O, so worker pool can be blocked.

True. I view the server case as more concerning because of the DoS possibility. Do you want me to remove this caveat?

Remove, or expand to make more clear the situations in which some things aren't problems. No need to optimize anything for a short-running app that does a one-shot HTTP request is what you are trying to say, I think.

sam-github · 2017-11-21T21:25:44Z

locale/en/docs/guides/dont-block-the-event-loop.md

+If you're writing command-line scripts with Node, this guide might still come in handy, but it isn't crucial.
+
+## TL; DR
+Node.js runs JavaScript code in the Event Loop and handles I/O in a Worker Pool (the threadpool).


No, just file I/O, network I/O is handled on the main loop.

Yes. I'll fix to clarify what kinds of I/O go to the Worker Pool.

sam-github · 2017-11-21T21:26:30Z

locale/en/docs/guides/dont-block-the-event-loop.md

+## TL; DR
+Node.js runs JavaScript code in the Event Loop and handles I/O in a Worker Pool (the threadpool).
+Node scales well, sometimes better than more heavyweight approaches like Apache.
+The secret to Node's scalability is that it uses a small number of threads to handle many clients.


where small number of threads is likely zero thread for a network server (no file i/o

This depends on the env variable and on whether the server has ever used the threadpool, since libuv's threads are created the first time the threadpool is used. I don't see a reason to confuse the issue though, this is intended as the high-level summary.

I feel this confuses the issue, though, because the secret to node's scalability isn't that it uses threadpools well (so does java), the secret is that a fairly typical HTTP server making outbound DB or other network requests uses no threads at all.

Right. It uses one thread -- the main loop -- to handle many clients, rather than Apache-style with one thread per active client.

sam-github · 2017-11-21T21:28:27Z

locale/en/docs/guides/dont-block-the-event-loop.md

+
+## A quick review of Node's architecture
+
+Node uses the Event-Driven Architecture: it has an Event Loop for orchestration and a Worker Pool for expensive tasks.


file i/o isn't expensive, no more so than network i/o, it just doesn't have good completion APIs at the O/S level to allow it to be done on the main loop, so threads are needed so that i/o can be done with blocking system calls

My understanding of the libuv loop (which is limited to Linux) is that the trouble is not the completion APIs but rather the ability of the OS to say that the file descriptor is ready and thus not to block when you do I/O on it. libuv only sends network traffic when it will not block in doing so, but the OS will not guarantee that read/write on a file descriptor will be non-blocking.

Based on this understanding, my claim is correct - the I/O done on the event loop is always cheap from the event loop's perspective because the OS guarantees it to be non-blocking.

Am I missing something here?

Maybe. The actual problem that is that you can't poll/epoll/select/kqueue a file descriptor for readiness, and so we can't do non-blocking reads or writes. And I guess you could call a blocking call "expensive", but network i/o can be done in thread pools with blocking calls, and lots of languages do exactly that. node doesn't, because network i/o can be non-blocking on unix, whereas file i/o can't (without the async i/o posix extensions, but they are poorly supported, and while windows has better async i/o for files, early tests by libuv authors showed that it would randomly block, it was to flaky to be useful).

Last I checked, posix async I/O is just a threadpool in libc, no different from the libuv threadpool. I've prototyped Node with kernel AIO to see if things had improved since 2011, but the O_DIRECT restriction remains and its costs are catastrophic.

Anyway, I'll flesh out this discussion a bit more.

aio was intended to be implemented in kernel and thus to rock... but hasn't happened yet, the saga is long, some info here: https://lwn.net/Articles/724198/. There is various user-space emulation using blocking I/O in a thread pool.... so, yeah, uv doesn't use that, there is no point.

sam-github · 2017-11-21T21:29:06Z

locale/en/docs/guides/dont-block-the-event-loop.md

+The Event Loop's queue is full of pending events.
+When the Event Loop sees a new event, it invokes its callback, which is JavaScript code that sees your Node application.
+The Event Loop is the only consumer of this queue.
+The Node runtime produces events for this queue.


the o/s produces the events, not the node runtime

Yes, via things like epoll. Where do you see the line between technical accuracy and "missing the forest for the trees" in guides like this?

because its not really a queue, if it was, we could ask "how long is that queue?", but we can't.... because it doesn't exist. node reads from the o/s's internal queue, gets a set of ready events, processes them then blocks reading again. no queue. the whole queue thing is brought up sometimes, but I don't think introducing something that doesn' exist is helpful in understanding how node works. I never describe anything like it in https://www.youtube.com/watch?v=P9csgxBgaZ8, for example

From a framework developer perspective, it's a queue that you can only see looking backwards. From an application developer perspective, though, I think the concept of a queue is a fine approximation of reality. I'm envisioning the target audience to be application developers for whom I think the "queue" concept is fine.

How strongly do you feel about this?

I am OK with the whole thing, except this sentence:

The Node runtime produces events for this queue.

I think that specific sentence should be removed. node doesn't produce the events. it asks for the o/s to produce them. I wouldn't say that I'm producing a cappucino when I order a cappucino at a cafe. Causing it to be produced? Sure, producing it? no.

sam-github · 2017-11-21T21:29:31Z

locale/en/docs/guides/dont-block-the-event-loop.md

+When the Event Loop sees a new event, it invokes its callback, which is JavaScript code that sees your Node application.
+The Event Loop is the only consumer of this queue.
+The Node runtime produces events for this queue.
+Specifically, Node asks libuv to monitor file descriptors using [epoll](http://man7.org/linux/man-pages/man7/epoll.7.html)), and the Event Loop's queue consists of the ready descriptors.


only on linux, its kqueue on OS X, and other mechanisms on Windows

Yes, good catch.

What do you think about saying "an epoll-like system call" with a link to the epoll man page? Part of my goal is to point users to resources that will give them a richer understanding of Node concepts and Node under the hood. That was also my intent with the "cooperative multitasking" link.

sam-github · 2017-11-21T21:31:10Z

locale/en/docs/guides/dont-block-the-event-loop.md

+The consumers of this queue are the Workers in the Worker Pool, who all compete to work on the next Task.
+Most of the time, the Event Loop is the only producer of Tasks for the Worker Pool.
+
+It is crucial to understand that this queue-based approach means that the Event Loop and the Workers rely on [cooperative multitasking](https://en.wikipedia.org/wiki/Cooperative_multitasking).


this isn't the common understanding of cooperative multi-tasking. coop tasking means that the running thread can suspend itself, but node doesn't do this, ever (on main loop or in pool). it runs to completion, which is not cooperative multitasking

I couldn't think of a better way to describe this other than saying "queue-based". Should I stick with that? Is there a better term?

Counterpoint, though -- queue-based is "cooperative multitasking" in the sense that a callback/task yields precisely when it completes.

I think that other than the queue to the uv work pool (which is actually a queue), I don't think queues should be mentioned. Not sure what a good replacement is, sorry! I can see the ghostly parallels, both node and co-op multitasking systems require careful consideration of how much work you do, and how you construct your app, but the analogy breaks down fast, because cooperative multi-tasking has a fairly agreed upon definition, and its not what node does, so better stay away from it, IMO.

sam-github · 2017-11-21T21:35:48Z

locale/en/docs/guides/dont-block-the-event-loop.md

+##### Best practices for offloading
+Follow these best practices for offloading:
+1. You should create your own Computation Worker Pool for extensive computation.
+2. The Computation Worker Pool should have no more than Workers than your machine has [logical cores](https://nodejs.org/api/os.html#os_os_cpus).


what about when you have multiple addons, each with a number of threads equal to the number of logical cores?

I would suggest creating a single Computation Worker Pool at the application level, similar to the Node Worker Pool that handles I/O from all points in the application. Do you have any reference applications I can look at to describe alternate patterns?

but no such thing exists, so current addons don't use it. it has been proposed that uv expose two work APIs, one for blocking non-cpu intensive calls (file i/o, dns.lookup), the other for cpu intensive activities (crypto.pbkdf for key generation, etc), but no code has been PRed, or benchmarks to show it will work. so, in the meantime, you have to consider all the internal pools of all the addons, and actually most addons will mix cpu and i/o intensive work in the same uv work pool. I'm slowly working towards getting metrics in node about the state of the uv pool, but that doesn't exist yet, so not worth mentioning here.

OK. If I added such an API would it be of interest? Is anyone working on this right now?

Also, I'll just remove this section. It sounds like this is too much of an open problem to worry about right now.

sam-github · 2017-11-21T21:36:52Z

locale/en/docs/guides/dont-block-the-event-loop.md

+### What is the Node Worker Pool used for?
+In the Node core modules, the Node Worker Pool is used for the asynchronous versions of the following classes of APIs:
+1. File system I/O
+2. DNS queries


only dns.lookup, the other ones don't use the pool

Yes. I'll fix this. I believe all of the FS APIs map directly to libuv versions, though?

https://nodejs.org/api/fs.html#fs_threadpool_usage - except fs.FSWatcher()

sam-github · 2017-11-21T21:40:21Z

locale/en/docs/guides/dont-block-the-event-loop.md

+Then this function would still be asynchronous, but the cost of each partition would be `O(n)`, not `O(1)`, making it much less safe to use for arbitrary values of `n`.
+
+## Conclusion
+Node has threads called Event Handlers, i.e. the Event Loop and the threadpool Workers, and these execute JS- and C++-callbacks.


the thread in the libuv work pool aren't called Event Handlers, except here, by you! And the lib uv workers never execute JS code. I don't think its helpful to overgeneralize the main thread to describe it as a kind of worker.

Yes, I agree that this is a weird overgeneralization. I'll take another look at this.

davisjam · 2017-11-21T22:17:57Z

Thanks for all the great comments, @sam-github! I'll get started on a revision.

Tiriel · 2017-11-21T22:24:45Z

Just a quick semi off-topic:
Thanks @sam-github & @davisjam , just reading your comments makes me learn a huge load of new things!
I really look forward to reading the final version of this guide!

- Rewrite the "A quick review of Node" section - Remove "Event Handler" abstraction - Provide more internal details - Relax the "Offloading best practices" - New task variation example: crypto.randomBytes - Misc. formatting and bonus material

davisjam · 2017-11-22T00:20:18Z

@sam-github e9c40e9 attempts to address your concerns.

a0viedo · 2017-11-22T13:41:36Z

locale/en/docs/guides/dont-block-the-event-loop.md

+This includes I/O for which an operating system does not provide a non-blocking version, as well as particularly CPU-intensive tasks.
+
+These are the Node module APIs that make use of this Worker Pool:
+1. I/O-intensive


worth mentioning net module?

It is my understanding that the net module does not use the Worker Pool, thanks to good OS support for non-blocking network I/O. The module docs do not bear the usual caveats about the threadpool, and as far as I can tell from the source there is no call to uv_queue_work.

Am I wrong in this? Or are you suggesting I explicitly mention the net module as a "safe" option? If so, I attempted to do so in the section "What code runs on the Event Loop?" with the sentence "The Event Loop will also fulfill the non-blocking asynchronous requests made by its callbacks, e.g., network I/O.". Do you think I should name a few examples there, like net and UDP?

correct, net does not use threads

sam-github · 2017-11-23T17:47:37Z

thanks for addressing my comments @davisjam

davisjam · 2017-11-23T19:21:41Z

Sure thing, @sam-github. Any more suggestions, or people that should be tagged?

sam-github · 2017-11-23T23:38:43Z

@jasnell was looking at loop behaviour, he might have some comments

benjamingr · 2017-11-25T14:16:58Z

locale/en/docs/guides/dont-block-the-event-loop.md

+Then this function would still be asynchronous, but the cost of each partition would be `O(n)`, not `O(1)`, making it much less safe to use for arbitrary values of `n`.
+
+## Conclusion
+Node has two types of threads: one Event Loop and `k` Workers.


What about the JavaScript thread?

I am referring to the "JavaScript thread" as the Event Loop throughout, to contrast it with the Worker Pool (threadpool) threads, and because it does more than JavaScript (e.g. wandering into libuv to fulfill network I/O requests, timers, etc.).

See the section "What code runs on the Event Loop?"

"In summary, the Event Loop executes the JavaScript callbacks registered for events, and is also responsible for fulfilling non-blocking asynchronous requests like network I/O."

Let me know if this is not clear or if you have alternative suggestions for phrasing.

benjamingr · 2017-11-25T14:26:46Z

locale/en/docs/guides/dont-block-the-event-loop.md

+#### Variation example: Long-running file system reads
+Suppose your server must read files in order to handle some client requests.
+After consulting Node's [File system](https://nodejs.org/api/fs.html) APIs, you opted to use `fs.readFile()` for simplicity.
+However, `fs.readFile()` is not partitioned: it submits a single `fs.read()` Task spanning the entire file.


This is actually being fixed if I understand correctly: nodejs/node#17054

That's the idea, anyway. But this guide is describing things as they are, not as I hope they will be.

See also nodejs/node#17250.

Might be worth adding a link to nodejs/node#17054 in here to note the proposed change then.

Added a link in 0a42816, thank you.

davisjam · 2017-11-27T15:56:04Z

I recognize this is a pretty long document, but does anyone have additional change requests?

gibfahn

This is really helpful, thanks a lot for writing this!

LGTM, couple of questions though.

gibfahn · 2017-12-04T10:53:16Z

locale/en/docs/guides/dont-block-the-event-loop.md

+`JSON.parse` and `JSON.stringify` are other potentially expensive operations.
+While these are `O(n)` in the length of the input, for large `n` they can take surprisingly long.
+
+If your server manipulates JSON objects, particularly those from a client, you should be cautious about the size of the objects or strings you work with on the Event Loop.


Is it worth mentioning asynchronous JSON parsing modules here (and then linking to The risks of npm modules section)?

Good idea, will do.

Addressed in ccd4f75.

gibfahn · 2017-12-04T10:54:52Z

locale/en/docs/guides/dont-block-the-event-loop.md

+
+In a server, *you should not use the synchronous APIs from these modules*.
+These APIs are expensive, because they involve significant computation or require I/O.
+If you execute them on the Event Loop, they will take far longer to complete than a typical JavaScript instruction, blocking the Event Loop.


What do you do if there is no async core api?

Maybe the answer is "raise an issue in core", but this was my question on reading this, so maybe worth saying something.

Thanks, I'll try another pass on this.

Addressed in ccd4f75.

gibfahn · 2017-12-04T12:58:40Z

locale/en/docs/guides/dont-block-the-event-loop.md

+##  The risks of npm modules
+Node developers benefit tremendously from the [npm ecosystem](https://www.npmjs.com/), with hundreds of thousands of modules offering functionality to accelerate your development process.
+
+However, the vast majority of these modules are written by third-party developers and undergo no formal verification process.


Does node core undergo a formal verification process? Mentioning it here makes it seem like it's in contrast to something else, but I'm not sure what that something is.

You're right, "formal" isn't the right word. It's more of a "many eyes on Node, few eyes on most 3rd-party modules" type situation. I'll change the wording.

Addressed in ccd4f75.

davisjam · 2017-12-04T23:48:33Z

@gibfahn Thank you for your review. I've tried to address your comments in ccd4f75. Let me know if you have more concerns before you'd be willing to approve.

davisjam · 2017-12-04T23:49:28Z

@sam-github Anything else needed for your approval?

sam-github · 2017-12-05T00:36:51Z

looks good to me

davisjam · 2017-12-07T16:34:38Z

The full draft has been up for about 2 weeks and has gone through a few reviews. Ready to be landed?

davisjam changed the title ~~WIP Guide on "Don't block the event loop"~~ Guide on "Don't block the event loop" Nov 19, 2017

davisjam mentioned this pull request Nov 19, 2017

"Don't block the event loop": New guide on avoiding DoS #1471

Closed

Tiriel reviewed Nov 19, 2017

View reviewed changes

WIP Guide on "Don't block the event loop"

eeb8dfe

For nodejs#1471

davisjam force-pushed the DontBlockTheEventLoop branch from 20a30d5 to eeb8dfe Compare November 19, 2017 22:51

davisjam added 2 commits November 19, 2017 18:43

More formal language, some examples

349dc72

Add indexOf

8091bc8

davisjam added 2 commits November 19, 2017 23:03

Edits and more examples

f56dffe

- REDOS - JSON DOS - "Complex calculations without blocking the Event Loop"

Typo

19b399c

davisjam added 2 commits November 20, 2017 10:57

Refresh sections: npm modules, conclusion

f526577

Don't Block the Worker Pool

7363536

Describe core module use of the Node Worker Pool

1bc444a

vsemozhetbyt added content Issues/pr concerning content enhancement labels Nov 21, 2017

sam-github reviewed Nov 21, 2017

View reviewed changes

Address review comments

e9c40e9

- Rewrite the "A quick review of Node" section - Remove "Event Handler" abstraction - Provide more internal details - Relax the "Offloading best practices" - New task variation example: crypto.randomBytes - Misc. formatting and bonus material

a0viedo reviewed Nov 22, 2017

View reviewed changes

benjamingr reviewed Nov 25, 2017

View reviewed changes

Add link to node/17054

0a42816

benjamingr approved these changes Nov 27, 2017

View reviewed changes

ghost approved these changes Dec 3, 2017

View reviewed changes

gibfahn reviewed Dec 4, 2017

View reviewed changes

Address review comments

ccd4f75

gibfahn approved these changes Dec 5, 2017

View reviewed changes

ghost merged commit 61e3ff1 into nodejs:master Dec 7, 2017

snyk-bot mentioned this pull request Nov 26, 2022

[Snyk] Upgrade autoprefixer from 10.4.1 to 10.4.13 NOUIY/nodejs.org#173

Open

snyk-bot mentioned this pull request Mar 21, 2023

[Snyk] Upgrade autoprefixer from 10.4.7 to 10.4.13 Woodpile37/nodejs.org#10

Merged

This pull request was closed.


		A safer alternative is the [node-re2](https://github.com/uhop/node-re2) project, which uses Google's blazing-fast [RE2](https://github.com/google/re2) regexp engine. But be warned, RE2 is not 100% compatible with Node's regexps, so check for regressions if you swap in the node-re2 module to handle your regexps. And really complicated regexps may not be supported by node-re2.

		As usual, if you're trying to match something "obvious", like a URL or a file path, use an npm module!


		## A quick review of Node's architecture

		Node uses the Event-Driven Architecture: it has an Event Loop for orchestration and a Worker Pool for expensive tasks.

Guide on "Don't block the event loop" #1478

Guide on "Don't block the event loop" #1478

Conversation

davisjam commented Nov 19, 2017

Tiriel left a comment

Choose a reason for hiding this comment

davisjam commented Nov 19, 2017

Tiriel 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

vsemozhetbyt commented Nov 19, 2017

davisjam commented Nov 19, 2017

davisjam commented Nov 19, 2017

Tiriel commented Nov 20, 2017

davisjam commented Nov 20, 2017

ayaankazerouni commented Nov 20, 2017

davisjam commented Nov 21, 2017

vsemozhetbyt commented Nov 21, 2017

gibfahn commented Nov 21, 2017

sam-github 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

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

davisjam Nov 21, 2017 • 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

davisjam commented Nov 21, 2017

Tiriel commented Nov 21, 2017

davisjam commented Nov 22, 2017

Choose a reason for hiding this comment

davisjam Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github commented Nov 23, 2017

davisjam commented Nov 23, 2017

sam-github commented Nov 23, 2017

Choose a reason for hiding this comment

davisjam Nov 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davisjam Nov 21, 2017 •

edited

Loading

davisjam Nov 22, 2017 •

edited

Loading

davisjam Nov 25, 2017 •

edited

Loading