IBM-Swift/Kitura#1143 Invoke socket delegate onAccept on a separate thread

[djones6]

Description

This PR decouples the invocation of delegate?.onAccept() from listenerSocket.acceptClientConnection(), invoking the former in a separate thread.

Note, this depends on Kitura/BlueSocket#85 and the subsequent related changes, tagged in BlueSocket 0.12.67.

Motivation and Context

Resolves Kitura/Kitura#1143

This avoids blocking the listener thread if a badly-behaved client connects to an SSL-enabled listener, which will block in SSL_accept until the client transmits an SSL handshake.

How Has This Been Tested?

I ran the Kitura-net tests (with Socket branch issue_1143) and they passed.

I tested with a Kitura-based application which uses SSLService, driven under load (50 concurrent clients from wrk) and the application functioned correctly. I tested on both Linux and Mac.

I also verified that, when creating a bad connection (as described above), the listener was no longer prevented from accepting further well-behaved connections.

Checklist:

• I have submitted a CLA form
• If applicable, I have updated the documentation accordingly.
• If applicable, I have added tests to cover my changes.

[youming-lin]

LGTM

[youming-lin]

@djones6
Consistent errors from Travis tests. Can you investigate?

[djones6]

@youming-lin I can recreate the failures locally - I'm puzzled as to why I didn't see these before... Investigating.

[djones6]

Okay, I think the reason I didn't see them before was that some additional checks have been added on top of my original PR for Socket: Kitura/BlueSocket@5c77668

This introduces a check to see whether the call to invokeDelegateOnAccept is appropriate. The errors being thrown during the tests are -9966 which is the new SOCKET_ERR_INVALID_DELEGATE_CALL. So the problem is apparently related to how we are setting the socket's delegate (perhaps these extra checks have highlighted a bug). Still investigating.

[billabt]

That exception is thrown under two conditions. The first is when the API is called with a Socket that was not returned by an acceptClientConnection(invokeDelegate: false). The second instance is when it's called multiple times for the same Socket.

[djones6]

The exception in this case is because the socket does not have a delegate. The original change I made to Socket allowed invokeDelegateOnAccept to be invoked regardless of whether a delegate was present (if delegate was nil, it simply did nothing).

The check does not allow this, as self.delegate is nil, and so needsAcceptDelegateCall is never set to true (regardless of how acceptClientConnection was called).

Kitura-net now needs to check whether the socket has a delegate before trying to invoke the new function.

[ianpartridge]

if listenSocket.delegate { } 🙂

[djones6]

Through fixing the bug above, I realized that I had added an async invocation for every socket accept, even if there is no delegate (which is unnecessary).

I pushed a refactoring that only goes down the async route if the socket has a delegate. I think it also makes it clearer which parts of the process the error handling applies to.

[codecov-io]

Codecov Report

Merging #219 into master will increase coverage by 0.24%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master     #219      +/-   ##
==========================================
+ Coverage   73.77%   74.02%   +0.24%     
==========================================
  Files          31       31              
  Lines        4122     4146      +24     
==========================================
+ Hits         3041     3069      +28     
+ Misses       1081     1077       -4

Flag	Coverage Δ
#CHTTPParser	56.11% <ø> (ø)	⬆️
#KituraNet	74.02% <83.33%> (+0.24%)	⬆️

Impacted Files	Coverage Δ
Sources/KituraNet/HTTP/HTTPServer.swift	80.89% <83.33%> (+0.37%)	⬆️
Sources/KituraNet/IncomingSocketHandler.swift	78.87% <0%> (+1.87%)	⬆️
...ces/KituraNet/Server/ServerLifecycleListener.swift	83.33% <0%> (+4.16%)	⬆️
Sources/KituraNet/IncomingSocketManager.swift	90.9% <0%> (+5.45%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b1b9b2...e285207. Read the comment docs.

[djones6]

I'm adding a test to cover the original bug.