Skip to content

Commit

Permalink
Kitura/Kitura#1143 Invoke socket delegate onAccept on a separate thre…
Browse files Browse the repository at this point in the history
…ad (#219)
  • Loading branch information
djones6 authored and tunniclm committed Oct 9, 2017
1 parent 5b1b9b2 commit b3b3c5d
Show file tree
Hide file tree
Showing 4 changed files with 232 additions and 29 deletions.
81 changes: 62 additions & 19 deletions Sources/KituraNet/HTTP/HTTPServer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -188,29 +188,19 @@ public class HTTPServer: Server {
private func listen(listenSocket: Socket, socketManager: IncomingSocketManager) {
repeat {
do {
let clientSocket = try listenSocket.acceptClientConnection()
let clientSocket = try listenSocket.acceptClientConnection(invokeDelegate: false)
Log.debug("Accepted HTTP connection from: " +
"\(clientSocket.remoteHostname):\(clientSocket.remotePort)")

#if os(Linux)
let negotiatedProtocol = clientSocket.delegate?.negotiatedAlpnProtocol ?? "http/1.1"
#else
let negotiatedProtocol = "http/1.1"
#endif

if let incomingSocketProcessorCreator = HTTPServer.incomingSocketProcessorCreatorRegistry[negotiatedProtocol] {
let serverDelegate = delegate ?? HTTPServer.dummyServerDelegate
let incomingSocketProcessor: IncomingSocketProcessor?
switch incomingSocketProcessorCreator {
case let creator as HTTPIncomingSocketProcessorCreator:
incomingSocketProcessor = creator.createIncomingSocketProcessor(socket: clientSocket, using: serverDelegate, keepalive: self.keepAliveState)
default:
incomingSocketProcessor = incomingSocketProcessorCreator.createIncomingSocketProcessor(socket: clientSocket, using: serverDelegate)
if let _ = listenSocket.delegate {
DispatchQueue.global().async { [weak self] in
if let strongSelf = self {
strongSelf.initializeClientConnection(clientSocket: clientSocket, listenSocket: listenSocket)
strongSelf.handleClientConnection(clientSocket: clientSocket, socketManager: socketManager)
}
}
socketManager.handle(socket: clientSocket, processor: incomingSocketProcessor!)
}
else {
Log.error("Negotiated protocol \(negotiatedProtocol) not supported on this server")
} else {
handleClientConnection(clientSocket: clientSocket, socketManager: socketManager)
}
} catch let error {
if self.state == .stopped {
Expand All @@ -236,6 +226,59 @@ public class HTTPServer: Server {
}
}

/// Initializes a newly accepted client connection.
/// This procedure may involve reading bytes from the client (in the case of an SSL handshake),
/// so must be done on a separate thread to avoid blocking the listener (Kitura issue #1143).
///
private func initializeClientConnection(clientSocket: Socket, listenSocket: Socket) {
do {
if let _ = listenSocket.delegate {
try listenSocket.invokeDelegateOnAccept(for: clientSocket)
}
} catch let error {
if self.state == .stopped {
if let socketError = error as? Socket.Error {
if socketError.errorCode == Int32(Socket.SOCKET_ERR_ACCEPT_FAILED) {
Log.info("Server has stopped listening")
} else {
Log.warning("Socket.Error accepting client connection after server stopped: \(error)")
}
} else {
Log.warning("Error accepting client connection after server stopped: \(error)")
}
} else {
Log.error("Error accepting client connection: \(error)")
self.lifecycleListener.performClientConnectionFailCallbacks(with: error)
}
}
}

/// Completes the process of accepting a new client connection. This is either invoked from the
/// main listen() loop, or in the presence of an SSL delegate, from an async block.
///
private func handleClientConnection(clientSocket: Socket, socketManager: IncomingSocketManager) {
#if os(Linux)
let negotiatedProtocol = clientSocket.delegate?.negotiatedAlpnProtocol ?? "http/1.1"
#else
let negotiatedProtocol = "http/1.1"
#endif

if let incomingSocketProcessorCreator = HTTPServer.incomingSocketProcessorCreatorRegistry[negotiatedProtocol] {
let serverDelegate = delegate ?? HTTPServer.dummyServerDelegate
let incomingSocketProcessor: IncomingSocketProcessor?
switch incomingSocketProcessorCreator {
case let creator as HTTPIncomingSocketProcessorCreator:
incomingSocketProcessor = creator.createIncomingSocketProcessor(socket: clientSocket, using: serverDelegate, keepalive: self.keepAliveState)
default:
incomingSocketProcessor = incomingSocketProcessorCreator.createIncomingSocketProcessor(socket: clientSocket, using: serverDelegate)
}
socketManager.handle(socket: clientSocket, processor: incomingSocketProcessor!)
}
else {
Log.error("Negotiated protocol \(negotiatedProtocol) not supported on this server")
}
}

/// Stop listening for new connections.
public func stop() {
self.state = .stopped
Expand Down
26 changes: 17 additions & 9 deletions Tests/KituraNetTests/KituraNetTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ class KituraNetTest: XCTestCase {
usingSelfSignedCerts: true, cipherSuite: nil)
#endif
}()

static let clientSSLConfig = SSLService.Configuration(withCipherSuite: nil, clientAllowsSelfSignedCertificates: true)

private static let initOnce: () = PrintLogger.use(colored: true)

Expand All @@ -54,22 +56,28 @@ class KituraNetTest: XCTestCase {
func doTearDown() {
}

func startServer(_ delegate: ServerDelegate?, port: Int = portDefault, useSSL: Bool = useSSLDefault) throws -> HTTPServer {

let server: HTTPServer
if useSSL {
server = HTTP.createServer()
server.delegate = delegate
server.sslConfig = KituraNetTest.sslConfig
try server.listen(on: port)
} else {
server = try HTTPServer.listen(on: port, delegate: delegate)
}
return server
}

func performServerTest(_ delegate: ServerDelegate?, port: Int = portDefault, useSSL: Bool = useSSLDefault,
line: Int = #line, asyncTasks: (XCTestExpectation) -> Void...) {

do {
self.useSSL = useSSL
self.port = port

let server: HTTPServer
if useSSL {
server = HTTP.createServer()
server.delegate = delegate
server.sslConfig = KituraNetTest.sslConfig
try server.listen(on: port)
} else {
server = try HTTPServer.listen(on: port, delegate: delegate)
}
let server: HTTPServer = try startServer(delegate, port: port, useSSL: useSSL)
defer {
server.stop()
}
Expand Down
151 changes: 151 additions & 0 deletions Tests/KituraNetTests/RegressionTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
/**
* Copyright IBM Corporation 2017
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
**/

import Foundation
import Dispatch

import XCTest

@testable import KituraNet
import Socket
import SSLService

class RegressionTests: KituraNetTest {

static var allTests : [(String, (RegressionTests) -> () throws -> Void)] {
return [
("testIssue1143", testIssue1143),
]
}

override func setUp() {
doSetUp()
}

override func tearDown() {
doTearDown()
}

/// Tests the resolution of Kitura issue 1143: SSL socket listener becomes blocked and
/// does not accept further connections if a 'bad' connection is made that then sends
/// no data (where the server is waiting on SSL_accept to receive a handshake).
///
/// The sequence of steps that cause a hang:
///
/// - A non-SSL client connects to SSL listener port, then does nothing
/// - On the server side, the listener thread expects an SSL client to be connecting.
/// It invokes the SSL delegate, goes into SSL_accept and then blocks, waiting for
/// an SSL handshake (which will never arrive)
/// - Another (well-behaved) SSL client attempts to connect. This hangs, because the
/// thread that normally loops around accepting incoming connections is still blocked
/// trying to SSL_accept the previous connection.
///
/// The fix for this issue is to decouple the socket accept from the SSL handshake, and
/// perform the latter on a separate thread. The expected behaviour is that a 'bad'
/// (non-SSL) connection does not interfere with the server's ability to accept other
/// connections.
func testIssue1143() {
do {
let server: HTTPServer = try startServer(nil, port: 0, useSSL: true)
defer {
server.stop()
}

guard let serverPort = server.port else {
XCTFail("Server port was not initialized")
return
}
XCTAssertTrue(serverPort != 0, "Ephemeral server port not set")

// Queue a server stop operation in 1 second, in case the test hangs (socket listener blocks)
let recoveryOperation = DispatchWorkItem {
server.stop()
XCTFail("Test did not complete (hung), server has been stopped")
}
DispatchQueue.global().asyncAfter(deadline: DispatchTime.now() + DispatchTimeInterval.seconds(1), execute: recoveryOperation)

let badClient = try BadClient()
let goodClient = try GoodClient()

defer {
badClient.disconnect()
goodClient.disconnect()
}

/// Connect a 'bad' (non-SSL) client to the server
try badClient.connect(serverPort)
XCTAssertEqual(badClient.connectedPort, serverPort, "BadClient not connected to expected server port")
XCTAssertFalse(badClient.socket.isSecure, "Expected BadClient socket to be insecure")

/// Connect a 'good' (SSL enabled) client to the server
try goodClient.connect(serverPort)
XCTAssertEqual(goodClient.connectedPort, serverPort, "GoodClient not connected to expected server port")
XCTAssertTrue(goodClient.socket.isSecure, "Expected GoodClient socket to be secure")

/// Test succeeded (did not hang)
recoveryOperation.cancel()

} catch {
XCTFail("Error: \(error)")
}
}

/// A simple client based on BlueSocket, which connects to a port but sends no data
struct BadClient {
let socket: Socket

var connectedPort: Int {
return Int(self.socket.remotePort)
}

init() throws {
socket = try Socket.create()
}

func connect(_ port: Int) throws {
try socket.connect(to: "localhost", port: Int32(port))
}

func disconnect() {
socket.close()
}

}

/// A simple client based on BlueSSLService, which connects to a port and performs
/// an SSL handshake
struct GoodClient {
let socket: Socket

var connectedPort: Int {
return Int(self.socket.remotePort)
}

init() throws {
socket = try Socket.create()
socket.delegate = try SSLService(usingConfiguration: clientSSLConfig)
}

func connect(_ port: Int) throws {
try socket.connect(to: "localhost", port: Int32(port))
}

func disconnect() {
socket.close()
}

}
}
3 changes: 2 additions & 1 deletion Tests/LinuxMain.swift
Original file line number Diff line number Diff line change
Expand Up @@ -71,5 +71,6 @@ XCTMain([
testCase(MonitoringTests.allTests.shuffled()),
testCase(ParserTests.allTests.shuffled()),
testCase(SocketManagerTests.allTests.shuffled()),
testCase(UpgradeTests.allTests.shuffled())
testCase(UpgradeTests.allTests.shuffled()),
testCase(RegressionTests.allTests.shuffled())
].shuffled())

0 comments on commit b3b3c5d

Please sign in to comment.