eagerly process input EOF/connection resets (fixes #277) #286

weissi · 2018-04-05T19:30:46Z

Motivation:

We had a number of problems:

We wanted to lazily process input EOFs and connection resets only
when the user actually calls read(). On Linux however you cannot
unsubscribe from EPOLLHUP so that's not possible.
Lazily processing input EOFs/connection resets wastes kernel
resources and that could potentially lead to a DOS
The very low-level Selector interpreted the eventing mechanism's
events quite a lot so the EventLoop/Channel only ever saw
readable or writable without further information what exactly
happened.
We completely ignored EPOLLHUP until now which on Unix Domain
Socket close leads to a 100% CPU spin (issue NIO mishandles EPOLLHUP #277)

Modifications:

made the Selector interface richer, it now sends the following
events: readable, writable, readEOF (input EOF), reset
(connection reset or some error)
process input EOFs and connection resets/errors eagerly
change all tests which relied on using unconnected and unbound sockets
to user connected/bound ones as epoll_wait otherwise would keep
sending us a stream of EPOLLHUPs which would now lead to an eager
close

Result:

most importantly: fixes NIO mishandles EPOLLHUP #277
waste less kernel resources (by dealing with input EOFs/connection
resets eagerly)
bring kqueue/epoll more in line

Lukasa

Cool, so these are mostly requests to clean up code that was messy before, but now that we're in the area we should make it nicer.

Lukasa · 2018-04-09T16:13:53Z

IntegrationTests/tests_01_http/test_19_connection_drop_while_waiting_for_response_uds.sh

+kill -0 $server_pid
+echo -e 'GET /dynamic/write-delay/10000 HTTP/1.1\r\n\r\n' | nc -w1 -U "$socket"
+sleep 0.2
+stop_server "$token"


Does this test actually "fail" in any particular way?

@Lukasa yes, in case it leaks fds or if it crashes. The old code wouldn't have passed this. I acknowledge this is not very obvious...

@weissi maybe also add a comment explain this ;) ?

@weissi please address this one ;)

Lukasa · 2018-04-09T16:19:00Z

Sources/NIO/BaseSocketChannel.swift

@@ -755,9 +751,36 @@ class BaseSocketChannel<T: BaseSocket>: SelectableChannel, ChannelCore {
        }
    }

+    final func readEOF() {
+        if self.lifecycleManager.isRegistered {
+            self.safeReregister(interested: self.interestedEvent.subtracting(.readEOF))


Can we add a comment to say why we subtract readEOF (namely, that we want this to be one-shot).

Lukasa · 2018-04-09T16:19:26Z

Sources/NIO/BaseSocketChannel.swift

+
+            while self.lifecycleManager.isActive {
+                var didReceiveEOF: Bool = false
+                self.readable0(didReceiveEOF: &didReceiveEOF)


Why does this use an inout instead of a return value?

@Lukasa/@normanmaurer because of the 'blind boolean' problem. We don't have named return values and I didn't really wanted to introduce a new enum just for that. Happy to change to whatever you both think is best.

I think adding an enum would be ok.

I'm perfectly fine with adding an enum for this. 😄

Lukasa · 2018-04-09T16:19:54Z

Sources/NIO/BaseSocketChannel.swift

+        if self.lifecycleManager.isRegistered {
+            self.safeReregister(interested: self.interestedEvent.subtracting(.readEOF))
+
+            while self.lifecycleManager.isActive {


Can we ever drop out of the loop due to a change in isActive without hitting didReceiveEOF?

@Lukasa yes, if the users closes in channelRead. Then we currently just discard the remaining ones. Adding a test.

@normanmaurer / @Lukasa is that what we want?

Lukasa · 2018-04-09T16:20:21Z

Sources/NIO/BaseSocketChannel.swift

+        }
+    }
+
+    // this _needs_ to synchronously cause the fd to be unregistered


Can you elaborate in this comment to say why?

Lukasa · 2018-04-09T16:35:28Z

Sources/NIO/Selector.swift

            default:
                // We only use EVFILT_USER, EVFILT_READ and EVFILT_WRITE.
                fatalError("unexpected filter \(ev.filter)")
            }
+            if selectorEvent != ._none {


Might be nicer to express this as a guard: guard selectorEvent != .none, let registration = registrations[Int(ev.ident)] else { continue }

Lukasa · 2018-04-09T16:36:19Z

Sources/NIO/Selector.swift

@@ -577,3 +618,20 @@ public enum IOEvent {
    /// Not interested in any event.
    case none
 }
+
+struct SelectorEventSet: OptionSet, Equatable {


Doc comment please. 😄

Lukasa · 2018-04-09T16:36:46Z

Sources/NIO/SocketChannel.swift

@@ -122,7 +122,7 @@ final class SocketChannel: BaseSocketChannel<Socket> {
        var result = ReadResult.none
        for i in 1...maxMessagesPerRead {
            guard self.isOpen && !self.inputShutdown else {
-                return result
+                throw ChannelError.eof


This is a surprising behavioural change: why was it made?

@Lukasa because I think it was wrong before. We would read EOF but we short-circuited it with something that isn't actually EOF.

Lukasa · 2018-04-09T16:37:41Z

Sources/NIOHTTP1Server/main.swift

@@ -424,7 +432,12 @@ private final class HTTPHandler: ChannelInboundHandler {
 }

 // First argument is the program path
-let arguments = CommandLine.arguments
+var arguments = CommandLine.arguments.dropFirst(0)


Presumably this is an attempt to coerce the type?

Mind writing that in a comment just so others can see why that was done?

Lukasa · 2018-04-09T16:39:00Z

Tests/NIOTests/ChannelTests.swift

            public func channelRead(ctx: ChannelHandlerContext, data: NIOAny) {
-                XCTFail("Should not be called as autoRead is false and we did not call read(), but received \(self.unwrapInboundIn(data))")
+                if !self.seenEOF {
+                    XCTFail("Should not be called before seeing the EOF as autoRead is false and we did not call read(), but received \(self.unwrapInboundIn(data))")


Is this right? Surely we expect to see the EOF after the read data, not before it.

you're right :)

normanmaurer · 2018-04-10T08:44:09Z

@weissi please ping when we should review again

BasThomas · 2018-04-10T09:50:36Z

FYI @weissi, if you change the fix issue #277 in the description to fixes #277, GitHub will link the pull request to the issue and automatically close it once this gets merged. :)

weissi · 2018-04-10T09:51:31Z

@BasThomas oh awesome, thanks. TIL

BasThomas · 2018-04-10T18:19:15Z

For reference: https://blog.github.com/2013-05-14-closing-issues-via-pull-requests/

weissi · 2018-04-13T10:41:10Z

Sources/NIO/Selector.swift

+            }
+
+            var index: Int = 0
+            for (event, filter) in [(KQueueEventFilterSet.read, EVFILT_READ), (.write, EVFILT_WRITE), (.except, EVFILT_EXCEPT)] {


note to @Lukasa & @normanmaurer . Just in case you were as worried as I was if Swift will optimise this array to be on the stack: It does. I wrote a test program:

import Darwin enum Bla: Int { case read = 0xcafe01 case write = 0xcafe02 case except = 0xcafe03 } @inline(never) func foo() -> (kevent, kevent, kevent) { var es: (kevent, kevent, kevent) = (kevent(), kevent(), kevent()) for (i, e) in [(Bla.read, EVFILT_READ), (.write, EVFILT_WRITE), (.except, EVFILT_EXCEPT)] { var ex: kevent = kevent() ex.filter = Int16(e) ex.flags = i == .read ? 0 : 1 ex.fflags = 0xdeadbe ex.data = i.rawValue if i == .read { es.0 = ex } if i == .write { es.1 = ex } if i == .except { es.2 = ex } } return es } print(foo())

which gets really nice assembly:

so the loop gets fully unrolled etc, awesome job Swift compiler!

same seems to happen in the real code. Couldn't spot any calls (except to initStackObject) or any loops

weissi · 2018-04-13T11:01:56Z

btw:

master is

info: 1000_reqs_1_conn: allocations not freed: 0
info: 1000_reqs_1_conn: total number of mallocs: 285416
info: 1_reqs_1000_conn: allocations not freed: 0
info: 1_reqs_1000_conn: total number of mallocs: 1703361

jw-277 is

info: 1000_reqs_1_conn: allocations not freed: 0
info: 1000_reqs_1_conn: total number of mallocs: 285399
info: 1_reqs_1000_conn: allocations not freed: 0
info: 1_reqs_1000_conn: total number of mallocs: 1687359

which suggests that we lost 6 allocations per connection (pair), ie. three allocs per connection. I cannot claim that I know where they came from but I guess it's good we didn't regress :)

Lukasa

Cool, really like this so far! Some notes inline.

Lukasa · 2018-04-13T10:57:10Z

Sources/NIO/BaseSocketChannel.swift

+                    // we could both be registered & active (if our channel support half-closure) or unregistered & inactive (if it doesn't).
+                    break loop
+                case .error:
+                    // we should be unregistered and inactive now (as `readable0` would've called close.


Nit: unbalanced parenthesis.

Lukasa · 2018-04-13T10:58:12Z

Sources/NIO/BaseSocketChannel.swift

@@ -774,9 +820,11 @@ class BaseSocketChannel<T: BaseSocket>: SelectableChannel, ChannelCore {
        do {
            try readFromSocket()
        } catch let err {
+            let isEOF: Bool


Better to just set this to a return code?

Lukasa · 2018-04-13T11:00:11Z

Sources/NIO/EventLoop.swift

+                }
+            }
+
+            if  ev.contains(.read) {


Nit: extra space in here.

Lukasa · 2018-04-13T11:00:39Z

Sources/NIO/EventLoop.swift

+            }
+
+            if ev.contains(.readEOF) {
+                channel.readEOF()


Should we do read or readEOF? Seems like we shouldn't need to do both.

Lukasa · 2018-04-13T11:00:56Z

Sources/NIO/Selector.swift

@@ -41,6 +41,192 @@ private extension Optional {
    }
 }

+/// Represents IO events NIO might be interested in. `SelectorEventSet` is used for two purposed: To express interest


Nit: purposes.

Lukasa · 2018-04-13T11:09:48Z

Sources/NIO/Selector.swift

+                    }
+                    if ev.events & Epoll.EPOLLHUP.rawValue != 0 || ev.events & Epoll.EPOLLERR.rawValue != 0 {
+                        selectorEvent.formUnion(.reset)
+                    }


Though actually I might think it should be an extension containing an initializer for SelectorEventSet, actually.

Lukasa · 2018-04-13T11:10:28Z

Sources/NIO/Selector.swift

+                if Int32(ev.flags) & EV_EOF != 0 {
+                    selectorEvent.formUnion(.readEOF)
+                }
+                if ev.fflags != 0 {


Is this a bit over broad?

don't think so, AFAIK matches EPOLLERR

Lukasa · 2018-04-13T11:10:40Z

Sources/NIO/Selector.swift

-                    try body((SelectorEvent(io: .write, registration: registration)))
+                selectorEvent.formUnion(.write)
+                if ev.fflags != 0 {
+                    selectorEvent.formUnion(.reset)


Similarly, is this over broad?

Lukasa · 2018-04-13T11:11:07Z

Sources/NIO/Selector.swift

+            guard selectorEvent != .none, let registration = registrations[Int(ev.ident)] else {
+                continue
+            }
+            if !registration.interested.contains(.readEOF) && selectorEvent.contains(.readEOF) {


A comment here explaining why we do this would be nice.

Lukasa · 2018-04-13T11:11:37Z

Sources/NIO/Selector.swift

@@ -564,6 +697,7 @@ enum SelectorStrategy {
 }

 /// The IO for which we want to be notified.
+@available(*, deprecated, message: "IOEvent was made public by accident, is no longer removed internally and will be removed with SwiftNIO 2.0.0")


Nit: "is no longer used internally"

normanmaurer · 2018-04-13T12:01:14Z

Sources/NIO/BaseSocketChannel.swift

+                switch self.readable0() {
+                case .eof:
+                    // on EOF we stop the loop and we're done with our processing for `readEOF`.
+                    // we could both be registered & active (if our channel support half-closure) or unregistered & inactive (if it doesn't).


nit: s/support/supports/ ?

normanmaurer · 2018-04-13T12:02:17Z

Sources/NIO/BaseSocketChannel.swift

+                    break loop
+                case .normal:
+                    // normal, note that there is no guarantee we're still active (as the user might have closed in callout)
+                    continue loop


nit: I think you could just make this break

@normanmaurer but then the code would be really hard to read, no? continue loop is really clear, so is break loop. But break meaning 'break the switch & continue the loop' is incredibly hard to spot.

honestly I dont think its harder to read but I dont care to much about if you change it or not... Thats why its a nit ;)

normanmaurer · 2018-04-13T12:04:51Z

Sources/NIO/Bootstrap.swift

@@ -385,13 +391,20 @@ public final class ClientBootstrap {
            let channel = try SocketChannel(eventLoop: eventLoop as! SelectableEventLoop, protocolFamily: protocolFamily)

            channelInitializer(channel).then {
-                channelOptions.applyAll(channel: channel)
+                return channelOptions.applyAll(channel: channel)


why we need the return ?

normanmaurer · 2018-04-13T12:05:11Z

Sources/NIO/Bootstrap.swift

            }.map {
-                channel
+                return channel


why the return is needed ?

normanmaurer · 2018-04-13T12:06:45Z

Sources/NIO/Selector.swift

@@ -41,6 +41,192 @@ private extension Optional {
    }
 }

+/// Represents IO events NIO might be interested in. `SelectorEventSet` is used for two purposed: To express interest
+/// in a given event set and for notifications about an IO event set that has occured.


nit: consider making the two different purposes bullet-pointers.

normanmaurer · 2018-04-13T12:07:54Z

Sources/NIO/Selector.swift

+
+    typealias RawValue = UInt8
+
+    let rawValue: UInt8


nit: Should this be let rawValue: RawValue ?

it's the same but changing it

normanmaurer · 2018-04-13T12:08:54Z

Sources/NIO/Selector.swift

+private struct KQueueEventFilterSet: OptionSet, Equatable {
+    typealias RawValue = UInt8
+
+    let rawValue: UInt8


nit: Should this be let rawValue: RawValue ?

normanmaurer · 2018-04-13T12:09:25Z

Sources/NIO/Selector.swift

+private struct EpollFilterSet: OptionSet, Equatable {
+    typealias RawValue = UInt8
+
+    let rawValue: UInt8


nit: Should this be let rawValue: RawValue ?

normanmaurer · 2018-04-13T12:09:56Z

Sources/NIO/Selector.swift

+///  - `output` corresponds to `EPOLLOUT`
+///  - `error` corresponds to `EPOLLERR`
+private struct EpollFilterSet: OptionSet, Equatable {
+    typealias RawValue = UInt8


Do you want to move the typealias out of the struct as you define the same multiple times.

no I can't, OptionSet requires it. I could remove that line entirely and rely on the inference but there'll be changes in that area so let's be explicit.

@weissi got it...

normanmaurer · 2018-04-13T12:16:18Z

Tests/NIOTests/ChannelTests.swift

+            do {
+                try clientChannel.writeAndFlush(buffer).wait()
+            } catch {
+                break


can you add a comment why its expected that we may see an error ?

it shouldn't thanks

Lukasa

I think I'm happy here, well done!

Lukasa · 2018-04-16T11:03:33Z

(Except the tests are broken obviously)

weissi · 2018-04-16T11:19:19Z

thanks @Lukasa hoping for #314 :)

normanmaurer · 2018-04-16T12:16:41Z

Sources/NIO/Bootstrap.swift

+                return channel.register().then {
+                    assert(channel.eventLoop.inEventLoop)
+                    return body(channel)
+                }


@weissi consider making a utility function for the register / body execution and also use it for register / bind above. Better to keep all the "special" code in one place

normanmaurer · 2018-04-16T12:19:54Z

Sources/NIO/Selector.swift

+        var filter: UInt32 = 0
+        let epollFilters = EpollFilterSet(selectorEventSet: self)
+        if epollFilters.contains(.error) {
+            filter |= Epoll.EPOLLERR.rawValue


@weissi just a note here... You always are registered to EPOLLERR so no need to do this imho:

http://man7.org/linux/man-pages/man2/epoll_ctl.2.html

@normanmaurer yes I know we're always registered but I thought it would be nice to still do it explicitly. But I'll replace it by a comment

oh hang on, sorry @normanmaurer . Wait, you're always registered for EPOLLERR ? I thought it's only EPOLLHUP.

you're right, I'll remove it

normanmaurer

Just a few nits... looks great otherwise. Good work

normanmaurer · 2018-04-16T12:22:24Z

Tests/NIOTests/ChannelTests.swift

+        for _ in 0..<20 {
+            XCTAssertNoThrow(try clientChannel.writeAndFlush(buffer).wait())
+        }
+        try clientChannel.close().wait()


XCTAssertNoThrow ?

normanmaurer · 2018-04-16T12:22:30Z

Tests/NIOTests/ChannelTests.swift

+
+        // Wait for 100 ms.
+        usleep(100 * 1000);
+        try serverChannel.close().wait()


XCTAssertNoThrow

normanmaurer · 2018-04-16T12:22:47Z

Tests/NIOTests/ChannelTests.swift

+        try clientChannel.writeAndFlush(buf).wait()
+        try clientChannel.writeAndFlush(buf).wait()
+        try clientChannel.close().wait()
+        try allDone.futureResult.wait()


Do we want to use XCTAssertNoThrow here ?

normanmaurer · 2018-04-16T15:49:28Z

Sources/NIO/Bootstrap.swift

+private extension Channel {
+    func registerAndDoSynchronously(_ body: @escaping (Channel) -> EventLoopFuture<Void>) -> EventLoopFuture<Void> {
+        // this is pretty delicate at the moment:
+        // `body` (which will `connect`) must be _synchronously_ follow `register`, otherwise in our current


@weissi nit: the comment is not really correct anymore as it is not only for connect.

normanmaurer · 2018-04-16T15:49:43Z

Sources/NIO/Bootstrap.swift

+        // `body` (which will `connect`) must be _synchronously_ follow `register`, otherwise in our current
+        // implementation, `epoll` will send us `EPOLLHUP`. To have it run synchronously, we need to invoke the
+        // `then` on the eventloop that the
+        // `register` will succeed.


nit: merge with previous line

Motivation: We had a number of problems: 1. We wanted to lazily process input EOFs and connection resets only when the user actually calls `read()`. On Linux however you cannot unsubscribe from `EPOLLHUP` so that's not possible. 2. Lazily processing input EOFs/connection resets wastes kernel resources and that could potentially lead to a DOS 3. The very low-level `Selector` interpreted the eventing mechanism's events quite a lot so the `EventLoop`/`Channel` only ever saw `readable` or `writable` without further information what exactly happened. 4. We completely ignored `EPOLLHUP` until now which on Unix Domain Socket close leads to a 100% CPU spin (issue apple#277) Modifications: - made the `Selector` interface richer, it now sends the following events: `readable`, `writable`, `readEOF` (input EOF), `reset` (connection reset or some error) - process input EOFs and connection resets/errors eagerly - change all tests which relied on using unconnected and unbound sockets to user connected/bound ones as `epoll_wait` otherwise would keep sending us a stream of `EPOLLHUP`s which would now lead to an eager close Result: - most importantly: fix issue apple#277 - waste less kernel resources (by dealing with input EOFs/connection resets eagerly) - bring kqueue/epoll more in line

Motivation: Since apple#286, we do eagerly detect connection resets which is good. To resolve the situation we first try to read everything off the socket. If something fails there, everything is good and the client gets the correct error. If that however doesn't lead to any errors, we would close the connection with `error: ChannelError.eof` which the client would then see for example on a connect that happened to get `ECONNRESET` or `ECONNREFUSED`. That's not right so these situations are fixed here. Modifications: - on reset, try to get the socket error if any - if no socket error was detected, we send a `ECONNRESET` because we know we have been reset but not why and didn't encounter any errors - write tests for these situations Result: connection resets that don't hit any other errors will now see the correct error

Motivation: PR apple#286 started processing EPOLLHUPs. Sadly epoll also immediately throws EPOLLHUP at you if you register a socket that isn't either connected or listening. This fixes two more instances of this problem. Modifications: made sure sockets that are registered are either listening or connected (and before we let the Selector wait for events) Result: tests should be more reliable

Motivation: PR #286 started processing EPOLLHUPs. Sadly epoll also immediately throws EPOLLHUP at you if you register a socket that isn't either connected or listening. This fixes two more instances of this problem. Modifications: made sure sockets that are registered are either listening or connected (and before we let the Selector wait for events) Result: tests should be more reliable

Motivation: Since apple#286, we do eagerly detect connection resets which is good. To resolve the situation we first try to read everything off the socket. If something fails there, everything is good and the client gets the correct error. If that however doesn't lead to any errors, we would close the connection with `error: ChannelError.eof` which the client would then see for example on a connect that happened to get `ECONNRESET` or `ECONNREFUSED`. That's not right so these situations are fixed here. Modifications: - on reset, try to get the socket error if any - if no socket error was detected, we send a `ECONNRESET` because we know we have been reset but not why and didn't encounter any errors - write tests for these situations Result: connection resets that don't hit any other errors will now see the correct error

Motivation: Since #286, we do eagerly detect connection resets which is good. To resolve the situation we first try to read everything off the socket. If something fails there, everything is good and the client gets the correct error. If that however doesn't lead to any errors, we would close the connection with `error: ChannelError.eof` which the client would then see for example on a connect that happened to get `ECONNRESET` or `ECONNREFUSED`. That's not right so these situations are fixed here. Modifications: - on reset, try to get the socket error if any - if no socket error was detected, we send a `ECONNRESET` because we know we have been reset but not why and didn't encounter any errors - write tests for these situations Result: connection resets that don't hit any other errors will now see the correct error

weissi force-pushed the jw-277 branch from 912b2f6 to 7e6d7e0 Compare April 9, 2018 15:22

weissi changed the title ~~[DO NOT MERGE] [WIP] very early preview of fix for #277~~ eagerly process input EOF/connection resets (fixes #277) Apr 9, 2018

weissi force-pushed the jw-277 branch 3 times, most recently from 46217d5 to 7f96b84 Compare April 9, 2018 16:33

weissi mentioned this pull request Apr 9, 2018

NIO mishandles EPOLLHUP #277

Closed

weissi requested review from Lukasa and normanmaurer April 9, 2018 16:37

Lukasa requested changes Apr 9, 2018

View reviewed changes

weissi force-pushed the jw-277 branch from 7f96b84 to 071c6ad Compare April 9, 2018 17:30

weissi force-pushed the jw-277 branch 2 times, most recently from f2513bd to a229eb3 Compare April 10, 2018 15:32

weissi force-pushed the jw-277 branch 2 times, most recently from 3459801 to 9bdfbfb Compare April 11, 2018 17:18

Lukasa added the 🔨 semver/patch No public API change. label Apr 12, 2018

weissi force-pushed the jw-277 branch 2 times, most recently from 14212d3 to c1bae21 Compare April 13, 2018 10:31

weissi commented Apr 13, 2018

View reviewed changes

Lukasa requested changes Apr 13, 2018

View reviewed changes

normanmaurer reviewed Apr 13, 2018

View reviewed changes

normanmaurer requested changes Apr 13, 2018

View reviewed changes

weissi force-pushed the jw-277 branch 4 times, most recently from dac57f7 to db2594c Compare April 13, 2018 17:59

weissi mentioned this pull request Apr 14, 2018

fix errors received when holding messages in HTTPServerPipelineHandler #314

Merged

Lukasa approved these changes Apr 16, 2018

View reviewed changes

normanmaurer reviewed Apr 16, 2018

View reviewed changes

normanmaurer requested changes Apr 16, 2018

View reviewed changes

weissi force-pushed the jw-277 branch 2 times, most recently from d06a5c1 to 20d7b83 Compare April 16, 2018 13:59

normanmaurer reviewed Apr 16, 2018

View reviewed changes

weissi force-pushed the jw-277 branch from e67176c to 6c13bda Compare April 16, 2018 17:13

normanmaurer approved these changes Apr 16, 2018

View reviewed changes

normanmaurer added this to the 1.5.0 milestone Apr 16, 2018

normanmaurer merged commit b109389 into apple:master Apr 16, 2018

weissi mentioned this pull request Apr 17, 2018

propagate socket errors on .reset #325

Merged

weissi mentioned this pull request Apr 17, 2018

improve test reliability regarding EPOLLHUP #326

Merged

Lukasa mentioned this pull request Apr 18, 2018

Synchronous connect errors do not properly unregister their selector. #322

Closed

eagerly process input EOF/connection resets (fixes #277) #286

eagerly process input EOF/connection resets (fixes #277) #286

Conversation

weissi commented Apr 5, 2018 • edited Loading

Lukasa 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

normanmaurer commented Apr 10, 2018

BasThomas commented Apr 10, 2018

weissi commented Apr 10, 2018

BasThomas commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weissi commented Apr 13, 2018

Lukasa 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

Lukasa left a comment

Choose a reason for hiding this comment

Lukasa commented Apr 16, 2018

weissi commented Apr 16, 2018

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

normanmaurer 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

weissi commented Apr 5, 2018 •

edited

Loading