Implement client-side support for sockets being closed gracefully by the server

[Joannis]

Fixing #91

[andreasley]

Good catch. I think throwing an error is not appropriate here. Maybe something like this instead:

if receivedBytes == 0 && maxBytes != 0 {
    // A return value of 0 indicates that the remote peer has performed
    // an orderly shutdown – unless 0 bytes have been requested.
    // In this case, this socket can be closed.
    try self.close()
    return Array()
}

Engine seems to close the Stream anyway if it's empty and will attempt to do so a second time after this patch, but I think it's more correct to handle this in the TCPSocket.

[Joannis]

I think try self.close() will throw too since I think you can't close a closed socket.

[andreasley]

True, but the error thrown from calling close() a second time is ignored in Engine and won't show up in the log (as opposed to when throwing from recv). Also, Engine only closes the socket if the parser detects an empty stream, which it never will if recv throws, so the connection continues to exist. Of course this could all be changed, but in my opinion, Engine shouldn't have to worry about socket details anyway.

[Joannis]

I'm not sure if Engine should be the main perspective here. I'm pretty sure there are many other libraries running socks, like MongoKitten for example, which are also suffering from closed connections.

EDIT: The perspective I feel should be assumed would be socks itself without breaking API.

[andreasley]

I agree.

Still: Is it appropriate to throw an error if the remote peer performed an orderly shutdown? We can always add a check in close() to prevent subsequent calls to socket_close().

[Joannis]

Does anyone else care to weigh in? @LoganWright @tannernelson maybe?

[loganwright]

Are there alternatives that

1. don't throw
2. still close properly in socks for dependent libs.

[Joannis]

Is it normal that the WatchingTests fail?

[andreasley]

@Joannis Not really. They used to work, but something (in Travis?) changed and I couldn't reproduce the error yet. :(

Edit: And now they worked again. It's fairly random and seems to be related to a premature release of the underlying GCD semaphore of the DispatchSource.

[loganwright]

@tannernelson @andreasley @Joannis this looks good to me, any last comments before I merge it?

[andreasley]

@LoganWright Maybe add a comment why the socket should be closed if we get a return value of 0 (see my first comment)? That's all. :)

Note: We'll have to update Engine slightly.

Note 2: https://github.com/vapor/socks/pull/92 introduces a check to prevent additional attempts to close the socket