Fixed state handling of HTTP client connections #2273

peterzandbergen · 2017-10-11T17:46:46Z

The _conn: TCPConnection was used before it was opened.
Added an extra state _ConnConnecting for proper handling.

Also fixed _ClientConnection not closing the connection
when not work needed to be done by calling _send_pending
after having received the body part of the HTTP response.

Added one trick pony http server for testing

The _conn: TCPConnection was used before it was opened. Added an extra state _ConnConnecting for proper handling. Also fixed _ClientConnection not closing the connection when not work needed to be done by calling _send_pending after having received the body part of the HTTP response.

The server gives full control over the response.

mfelsche · 2017-10-11T19:32:11Z

packages/net/http/_test.pony

+  fun ref apply(h: TestHelper) ? =>
+    let urls: Array[URL] = 
+      [ 
+        URL.build(


testing the connection once one should be enough, right?

It is essential that the request is made more than once.

This is the flow of the old code before the fix:

be apply is called with payload 1

apply pushes payload 1 on the _unsent list

_send_pending is called

_send_pending tries to cast _conn as TCPConnection and fails, because _conn is None; it calls _new_conn()

_new_conn creates a new TCPConnection and assigns it to _conn

be apply returns

be apply is called with payload 2

apply pushes the payload on the _unsent list which now contains two payloads

apply calls _send_pending

_send_pending send_pending tries to cast _conn as TCPConnection and succeeds, but _conn is not open yet

_send_pending continues to shift the payloads from the _unsent list and calls request.write, which it thinks succeeds.

when all the request have been processed _send_pending and apply return and some time after that the be _connected behaviour is called and now _conn is really open and ready for use.

It works when you only send one payload, because _send_pending is called again after be _connected is called and _conn is a connected TCPConnection.

This is the scenario:

be apply is called with payload 1

apply pushes payload 1 on the _unsent list

_send_pending is called

_send_pending tries to cast _conn as TCPConnection and fails, because _conn is None; it calls _new_conn()

_new_conn creates a new TCPConnection and assigns it to _conn

be _connected is called and calls _send_pending

_send_pending works fine now, because _conn is actually connected.

Totally true. You need to send at least two requests.
But it would suffice to create 1 url, as it is always the same and just use a collections.Range from 0 to 5 or so to iterate over and send a request on each iteration.

That is true, but in my initial test I sent multiple request to different sites and this code made it easier to perform these tests.

I kept it in because there are more bugs in the http package and I may need it later. But I can take it out if you like.

Just to verify. Will the CI start a new build when I push new commits to the PR branch?

One idea still. Sorry if i seem picky but i did this wrong too many times in the past.

If you bind to port 50000 it might be already taken on someones machine and thus the testsuite might fail although pony is just fine.

My suggestion is to bind to port 0 with the TCPListener and to get the randomly assigned free port back in the TCPListenNotify.connected method from TCPListener.local_address().port. This way you make sure that you will most likely always get a free port and the test suite runs just fine.

Afaik the Ci will build every commit you make to a branch that has a PR open in the ponc repo

Will do that. Good tip. Did not know the feature yet.

I assume that you mean the TCPListenNotify.listening method.
The TCPListener.local_address().port returns a strange value. When I set the port value when creating TCPListener to e.g. 50000, then the port has value 20675, when I set it to 5000 port has value 34835. I have no idea why this is. But I assume it is undesired behaviour.

i sure mean TCPListenNotify.listening.
sorry for the confusion here.

The port from net.NetAddress is actually from the OS sockaddr_in (sockaddr_in6) structure defined in C. This is an unsigned short (U16) in network byte order that needs to be transformed to host byte order before using it. I learned that right now, sorry for that. :)
Here you have a playground link that shows this:

http://playground.ponylang.org/?gist=f783b91faedef2715d4a585f59ec4039

looking forward to merge your PR.

Theodus · 2017-10-11T19:45:56Z

packages/net/http/_client_connection.pony

@@ -98,7 +99,7 @@ actor _ClientConnection is HTTPSession
        if node()? is request then
          try (_conn as TCPConnection).dispose() end
          _conn = None
-          node.pop()?
+          node.>remove().pop()?


@peterzandbergen would you mind putting spaces around the .> operator to match the style guide?

will fix it

peterzandbergen · 2017-10-11T20:46:24Z

I must have missed that one. Will fix it and try again. Peter Zandbergen Tel: +31 6 460 55 872 linkedIn: peterzandbergen <https://www.linkedin.com/in/peterzandbergen>

…

On 11 October 2017 at 21:46, Theo Butler ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/net/http/_client_connection.pony <#2273 (comment)>: > @@ -98,7 +99,7 @@ actor _ClientConnection is HTTPSession if node()? is request then try (_conn as TCPConnection).dispose() end _conn = None - node.pop()? + node.>remove().pop()? @peterzandbergen <https://github.com/peterzandbergen> would you mind putting spaces around the .> operator to match the style guide? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2273 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI2XrraXgATGHsemCTtgEgzoFRpNPUHjks5srRsDgaJpZM4P13k5> .

peterzandbergen · 2017-10-11T20:50:28Z

Well Matthias, it wouldn't. The old code works well if the client submits only one request to the same scheme, host and service combination. I can send a small description of what happened in the old code. I discovered the bug when I sent two requests to the same web site. I admit that the number of request I put in is a bit over the top, but there should be more than one. I was playing chaos monkey. Cheers Peter Zandbergen Tel: +31 6 460 55 872 linkedIn: peterzandbergen <https://www.linkedin.com/in/peterzandbergen>

…

On 11 October 2017 at 21:32, Matthias Wahl ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/net/http/_test.pony <#2273 (comment)>: > + ha = ha' + h = h' + + fun apply(session: HTTPSession): HTTPHandler ref^ => + h.log("_TestHTTPHandlerFactory.apply called") + _TestHTTPHandler(ha, h) + +class iso _HTTPConnTest is UnitTest + var server: (TCPListener | None) = None + fun name(): String => "net/http/_HTTPConnection._new_conn" + fun label(): String => "conn-fix" + + fun ref apply(h: TestHelper) ? => + let urls: Array[URL] = + [ + URL.build( testing the connection once one should be enough, right? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2273 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI2XrmoOwifqqU2DhmaE_0AgUr4U6LO0ks5srRfCgaJpZM4P13k5> .

- put spaces around .> according to style guide - reduced the number of http requests to 3

mfelsche · 2017-10-11T21:42:21Z

packages/net/http/_test.pony

+    try (server as TCPListener).dispose() end
+
+////////////////////////
+primitive ResponseWriter


Please also make sure that all test types are private, that is start with underscore, otherwise they are exposed in the stdlib.

Needed to use the name() method of the NetAddress to retrieve the service number.

peterzandbergen · 2017-10-12T12:08:50Z

No worries. Just committed the working version. Op do 12 okt. 2017 10:21 schreef Matthias Wahl <[email protected]>:

***@***.**** commented on this pull request. ------------------------------ In packages/net/http/_test.pony <#2273 (comment)>: > + ha = ha' + h = h' + + fun apply(session: HTTPSession): HTTPHandler ref^ => + h.log("_TestHTTPHandlerFactory.apply called") + _TestHTTPHandler(ha, h) + +class iso _HTTPConnTest is UnitTest + var server: (TCPListener | None) = None + fun name(): String => "net/http/_HTTPConnection._new_conn" + fun label(): String => "conn-fix" + + fun ref apply(h: TestHelper) ? => + let urls: Array[URL] = + [ + URL.build( i sure mean TCPListenNotify.listening. sorry for the confusion here. The port from net.NetAddress is actually from the OS sockaddr_in ( sockaddr_in6) structure defined in C. This is an unsigned short (U16) in network byte order. I learned that right now, sorry for that. :) Here you have a playground link that shows this: http://playground.ponylang.org/?gist=f783b91faedef2715d4a585f59ec4039 looking forward to merge your PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2273 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI2Xrs2aQ2toZFPBjm_38xK4uJvpofgaks5srcwggaJpZM4P13k5> .

-- Peter Zandbergen Tel: +31 6 460 55 872 linkedIn: peterzandbergen <https://www.linkedin.com/in/peterzandbergen>

Praetonus · 2017-10-12T14:38:54Z

I've cancelled the AppVeyor builds since they were failing after a 1 hour time out. Not really sure what's going on here, maybe some firewall issues with AppVeyor.

peterzandbergen · 2017-10-12T15:27:36Z

Hi Benoit, I've noticed it too. The execution stops somewhere in the middle of a test, which does not seem to be related to my hacking. Cheers Peter Zandbergen Tel: +31 6 460 55 872 linkedIn: peterzandbergen <https://www.linkedin.com/in/peterzandbergen>

…

On 12 October 2017 at 16:39, Benoit Vey ***@***.***> wrote: I've cancelled the AppVeyor builds since they're failing after a 1 hour time out. Not really sure what's going on here, maybe some firewall issues with AppVeyor. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2273 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI2Xrr212zYVwsmVQCW7L-jDtvHN67K0ks5sriSEgaJpZM4P13k5> .

jemc · 2017-10-12T16:11:09Z

packages/net/http/_test.pony

+  //     "Content-Length: 0"
+  //     "Status: 520 Unknown Error"
+  //     ""
+  //   ]


What's going on with this commented-out code block?

jemc · 2017-10-12T16:11:24Z

packages/net/http/_test.pony

+  fun ref tear_down(h: TestHelper) =>
+    try (server as TCPListener).dispose() end
+
+////////////////////////


What's the purpose of this comment line?

peterzandbergen · 2017-10-12T16:48:35Z

Hi Joe, this can go, I kept it in because I thought I might need it later on to have different test cases for the _HTTPParser which I believe has errors. Cheers Peter Zandbergen Tel: +31 6 460 55 872 linkedIn: peterzandbergen <https://www.linkedin.com/in/peterzandbergen>

…

On 12 October 2017 at 18:11, Joe Eli McIlvain ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/net/http/_test.pony <#2273 (comment)>: > + // "Content-Type: text/html; charset=utf-8" + "Content-Length: 0" + "Status: 200 OK" + "" + ] + // For future use. + // fun val error_503(): Array[String val] val^ => + // [ as String val: + // "HTTP/1.1 520 Unknown Error" + // "Server: pony_fake_server" + // // "Date: Wed, 11 Oct 2017 15:16:32 GMT" + // // "Content-Type: text/html; charset=utf-8" + // "Content-Length: 0" + // "Status: 520 Unknown Error" + // "" + // ] What's going on with this commented-out code block? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2273 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI2XrqPEZ40cJndKB-sOCW7-glDHUy-iks5srjokgaJpZM4P13k5> .

peterzandbergen · 2017-10-12T16:49:13Z

Just a separator. Will fix it. Peter Zandbergen Tel: +31 6 460 55 872 linkedIn: peterzandbergen <https://www.linkedin.com/in/peterzandbergen>

…

On 12 October 2017 at 18:11, Joe Eli McIlvain ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/net/http/_test.pony <#2273 (comment)>: > + h, + {(p: String val) => + worker.listening(p) + } + ), + "", // all interfaces + "0" // service + ) + + // Start a long test for 2 seconds. + h.long_test(10_000_000_000) + + fun ref tear_down(h: TestHelper) => + try (server as TCPListener).dispose() end + +//////////////////////// What's the purpose of this comment line? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2273 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI2XrjkyiUebQyzEoFfkERpEQiNmPyMkks5srjo0gaJpZM4P13k5> .

Support classes for the client are prefixed with _HTTPConnTest. Fake server uses less classes, preventing name clashes.

mfelsche · 2017-10-13T22:55:53Z

This is most likely not a firewall issue on appveyor. There are also other tests in the net package that create a TCPListener e.g. https://github.com/ponylang/ponyc/blob/master/packages/net/_test.pony#L159

Maybe it is the TestHelper.dispose_when_done call that keeps resources hanging?
Just a wild guess tbh. But this is the only difference i can spot.

I would nonetheless suggest the decrease the test timeout significantly, so timeouts are much earlier detectable.

peterzandbergen · 2017-10-14T05:37:55Z

The timeout I use is 10 seconds. What do you advise? Op za 14 okt. 2017 00:56 schreef Matthias Wahl <[email protected]>:

This is most likely not a firewall issue on appveyor. There are also other tests in the net package that create a TCPListener e.g. https://github.com/ponylang/ponyc/blob/master/packages/net/_test.pony#L159 Maybe it is the TestHelper.dispose_when_done call that keeps resources hanging? Just a wild guess tbh. But this is the only difference i can spot. I would nonetheless suggest the decrease the test timeout significantly, so timeouts are much earlier detectable. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2273 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI2XrlRA_iR1E1c-mYAEALDd0KQqQ-8Qks5sr-qBgaJpZM4P13k5> .

-- Peter Zandbergen Tel: +31 6 460 55 872 linkedIn: peterzandbergen <https://www.linkedin.com/in/peterzandbergen>

mfelsche · 2017-10-14T06:30:49Z

Gosh, this is getting out of hand. Now i mistook the timings for milliseconds. I always do tbh.
Keep it this way.

I also meant that you should call TestHelper.dispose_when_done(...) for the same resources the mentioned test from the net package calls it. I.e. TCPListener

Appveyor times out on the test and the suspicion is that the http serve for generating the fixed response should be closed using dispose_when_done. This works for the test for TCPConnection.

NOT FOR MERGE.

mfelsche · 2017-10-16T18:22:10Z

I restartet 2 test runs on travisci on OSX that seem to be unrelated but concerning.

peterzandbergen · 2017-10-16T18:47:55Z

Hi Matthias, I was wondering what is special about my test code. I thought that it was allowed to instantiate actors in a unit test. Is there anything strange in my code that you can spot? Op ma 16 okt. 2017 20:22 schreef Matthias Wahl <[email protected]>:

I restartet 2 test runs on OSX that seem to be unrelated but concerning. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2273 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI2XrplkYWjff9baCITfL45DQhRtzyXIks5ss57cgaJpZM4P13k5> .

-- Peter Zandbergen Tel: +31 6 460 55 872 linkedIn: peterzandbergen <https://www.linkedin.com/in/peterzandbergen>

mfelsche

@peterzandbergen i am not 100% sure what the issue is with those tests. I hope the comments i made might help. it is most likely a socket not properly closed as the appveyor builds seem to be hanging after the test has been completed successfully.

mfelsche · 2017-10-16T21:24:54Z

packages/net/http/_test.pony

+    h = h'
+
+  fun apply(session: HTTPSession): HTTPHandler ref^ =>
+    h.log("_HTTPConnTestHandlerFactory.apply called")


You could also call TestHelper.dispose_when_done(session) here instead of adding a dispose() to the HTTPClient, although i think HTTPClient should have such a method. But rather in another PR.

mfelsche · 2017-10-16T21:34:41Z

packages/net/http/_test.pony

+              None
+
+            fun ref connect_failed(conn: TCPConnection ref) =>
+              None


i personally like the approach of https://github.com/ponylang/ponyc/blob/master/packages/net/_test.pony#L202 where multiple TestHelper.expect_action calls are made for each stage towards completion of the test. At points like this, where the test failed, a TestHelper.fail_action call can greatly help pinpointing possible issues.

mfelsche · 2017-10-16T21:35:36Z

packages/net/http/_test.pony

+                // Write the response.
+                if start then
+                  for r in response.values() do
+                    conn.write(r + "\n")


if i am correct, HTTP headers are separated by \r\n.

mfelsche · 2017-10-16T21:42:40Z

packages/net/http/_test.pony

+            try
+              (client as HTTPClient iso)(consume payload, hf)?
+            end
+            // match client


mfelsche · 2017-10-16T21:44:38Z

packages/net/http/_test.pony

+              end // while
+              true
+
+            fun ref accepted(conn: TCPConnection ref) =>


maybe a TestHelper.dispose_when_done(conn) on this side helps as well.

mfelsche · 2017-11-02T21:48:23Z

This one is blocked by #2322

chalcolith · 2017-11-07T00:53:06Z

Windows tests are fixed by #2325

peterzandbergen · 2017-11-07T06:39:17Z

Thanks for fixing the Windows issue. Does this mean that I can push my pull request again?

peterzandbergen · 2017-11-07T08:15:38Z

The test passes with the fix that Gordon made for the TCPConnection. So after his pull request #2325 has been accepted I will push my fix again for testing on CI.

jemc · 2017-11-16T21:54:27Z

#2325 has been merged.

peterzandbergen · 2017-11-17T16:06:01Z

One of the iOS tests in Travis failed because bintray get failed. Will bump a new version this evening to try again.

mfelsche

Finally! :)
Just some few final-cleanup remarks.

Great stuff!

mfelsche · 2017-11-17T20:37:57Z

packages/net/http/_client_connection.pony

@@ -178,6 +180,7 @@ actor _ClientConnection is HTTPSession
    `_chunk`. This is passed on to the front end.
    """
    _app_handler.finished()
+    _send_pending()


could you explain why this call is here?
I cannot see how this is related to the _conn state handling.
Isn't this method is called by the HTTPParser when a chunked response body has been fully received?

Hi Matthias,
this allows the state machine to handle the situation that no both pending and unsent requests are empty. If this line is removed then the connection will not close the connection if no work has to be done, will not dispose the connection and waits and waits .... I realize that this is an extra fix but needed it to get the tests working and not timing out.

mfelsche · 2017-11-17T20:43:25Z

packages/net/http/_client_connection.pony

@@ -189,9 +192,11 @@ actor _ClientConnection is HTTPSession
    """
    Close the connection from the client end.
    """
+    _cancel_all()


The behaviour of cancelling all pending requests should be documented in the docstring.

Will add it.

Hi Matthias, I processed your remarks and pushed a new commit.

mfelsche · 2017-11-17T20:43:45Z

packages/net/http/_test.pony

@@ -25,6 +29,8 @@ actor Main is TestList
    test(_Valid)
    test(_ToStringFun)

+    // Disabled to check if this causes the appveyor block.


mfelsche · 2017-11-17T20:45:12Z

packages/net/http/client.pony

@@ -53,6 +53,11 @@ class HTTPClient
    let valrequest: Payload val = consume request
    session(valrequest)
    valrequest
+
+  fun dispose() =>


please clear the _sessions map as well and add a docstring describing what happens here.

mfelsche · 2017-11-17T20:49:57Z

packages/net/http/_test.pony

+        end // try
+
+      be dispose() =>
+        // try


do you want to uncomment those lines?

I have to take it back. This was something that I put in to fix the problem on windows. Because it is not needed I will remove it from the code. As you mentioned earlier only one fix per pull request.

This code is not necessary. I put it in hoping it would fix the problem on Windows.

I will take it out.

mfelsche · 2017-11-17T20:54:30Z

I took the freedom to change the title to be more meaningful when appearing in the changelog as "Fixed" entry.

Processed the remarks from Matthias (thanks) - Added docstrings - cleared the _sessions in HTTPClient.dispose()

mfelsche

Approved.

peterzandbergen · 2017-11-21T05:34:08Z

Great. Thanks Op ma 20 nov. 2017 22:54 schreef Matthias Wahl <[email protected]>:

***@***.**** approved this pull request. Approved. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2273 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI2Xrs09ydziFxQAg5xQv4-tz0rbIXtUks5s4fURgaJpZM4P13k5> .

-- Peter Zandbergen Tel: +31 6 460 55 872 linkedIn: peterzandbergen <https://www.linkedin.com/in/peterzandbergen>

jemc · 2017-11-21T16:14:12Z

@mfelsche - I haven't reviewed this, but if you feel comfortable merging it, feel free to go ahead!

mfelsche · 2017-11-21T17:00:59Z

@jemc as far as i can see, this is totally fine. If it is not and i didnt see it, i take full responsibility and buy a round for everybody involved

peterzandbergen · 2017-11-21T18:01:16Z

I will share the round with you. No worries.

peterzandbergen added 2 commits October 8, 2017 23:03

Added one trick pony http server for testing

115bc7e

The server gives full control over the response.

peterzandbergen mentioned this pull request Oct 11, 2017

Fixed _conn state handling and closing connection #2263

Closed

mfelsche reviewed Oct 11, 2017

View reviewed changes

Theodus reviewed Oct 11, 2017

View reviewed changes

Processed comments on last commit and push

065cc3b

- put spaces around .> according to style guide - reduced the number of http requests to 3

mfelsche reviewed Oct 11, 2017

View reviewed changes

peterzandbergen added 2 commits October 12, 2017 10:54

WIP

818e2f6

Implemented randomly assigned service number for the fake http server

b7c895b

Needed to use the name() method of the NetAddress to retrieve the service number.

jemc reviewed Oct 12, 2017

View reviewed changes

Changed naming for supporting classes for _HTTPConnTest

24e0aea

Support classes for the client are prefixed with _HTTPConnTest. Fake server uses less classes, preventing name clashes.

peterzandbergen added 3 commits October 15, 2017 23:17

Replaced tear_down with dispose_when_done for Appveyor problem

86add4d

Appveyor times out on the test and the suspicion is that the http serve for generating the fixed response should be closed using dispose_when_done. This works for the test for TCPConnection.

Disabled the HTTP conn test to check if this causes appveyor to block.

4271075

NOT FOR MERGE.

Added dispose to http client to clean up tcpconnections.

63b958c

Added cancel all to dispose.

adef88d

mfelsche reviewed Oct 16, 2017

View reviewed changes

mfelsche mentioned this pull request Nov 2, 2017

TCPConnection close/dispose not working on Windows #2322

Closed

mfelsche added the bug: 0 - blocked label Nov 2, 2017

Updated test from fix-1 branch

a83f748

Added comment to create a new commit and push

de27102

mfelsche reviewed Nov 17, 2017

View reviewed changes

mfelsche added changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge and removed bug: 0 - blocked labels Nov 17, 2017

mfelsche changed the title ~~Fixed _conn state handling and closing connection~~ Fixed state handling of HTTP client connections Nov 17, 2017

Commented and cleaned up the code

f4edb90

Processed the remarks from Matthias (thanks) - Added docstrings - cleared the _sessions in HTTPClient.dispose()

mfelsche approved these changes Nov 20, 2017

View reviewed changes

jemc merged commit 06f3f65 into ponylang:master Nov 21, 2017

ponylang-main added a commit that referenced this pull request Nov 21, 2017

Update CHANGELOG for PR #2273 [skip ci]

9c912ee

Fixed state handling of HTTP client connections #2273

Fixed state handling of HTTP client connections #2273

Conversation

peterzandbergen commented Oct 11, 2017

Choose a reason for hiding this comment

peterzandbergen Oct 11, 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

peterzandbergen Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

mfelsche Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterzandbergen commented Oct 11, 2017 via email

peterzandbergen commented Oct 11, 2017 via email

Choose a reason for hiding this comment

peterzandbergen commented Oct 12, 2017 via email

Praetonus commented Oct 12, 2017 • edited Loading

peterzandbergen commented Oct 12, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterzandbergen commented Oct 12, 2017 via email

peterzandbergen commented Oct 12, 2017 via email

mfelsche commented Oct 13, 2017

peterzandbergen commented Oct 14, 2017 via email

mfelsche commented Oct 14, 2017 • edited Loading

mfelsche commented Oct 16, 2017 • edited Loading

peterzandbergen commented Oct 16, 2017 via email

mfelsche 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

mfelsche commented Nov 2, 2017

chalcolith commented Nov 7, 2017

peterzandbergen commented Nov 7, 2017

peterzandbergen commented Nov 7, 2017

jemc commented Nov 16, 2017

peterzandbergen commented Nov 17, 2017

mfelsche 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

mfelsche commented Nov 17, 2017

mfelsche left a comment

Choose a reason for hiding this comment

peterzandbergen commented Nov 21, 2017 via email

jemc commented Nov 21, 2017

mfelsche commented Nov 21, 2017

peterzandbergen commented Nov 21, 2017

peterzandbergen Oct 11, 2017 •

edited

Loading

peterzandbergen Oct 11, 2017 •

edited

Loading

mfelsche Oct 12, 2017 •

edited

Loading

Praetonus commented Oct 12, 2017 •

edited

Loading

mfelsche commented Oct 14, 2017 •

edited

Loading

mfelsche commented Oct 16, 2017 •

edited

Loading