From c88223ae372a33ed3cb553906b3cd4108e6049b5 Mon Sep 17 00:00:00 2001 From: David Jones Date: Tue, 8 Jan 2019 14:04:40 +0000 Subject: [PATCH] Swift 5: resolve Linux crash with concurrent access to socketHandlers (#284) --- .travis.yml | 6 ++ Sources/KituraNet/IncomingSocketManager.swift | 86 ++++++++++--------- Tests/KituraNetTests/SocketManagerTests.swift | 8 +- testWithGCD.sh | 2 + 4 files changed, 58 insertions(+), 44 deletions(-) create mode 100755 testWithGCD.sh diff --git a/.travis.yml b/.travis.yml index 9b138dd..c7acf17 100644 --- a/.travis.yml +++ b/.travis.yml @@ -32,6 +32,12 @@ matrix: sudo: required services: docker env: DOCKER_IMAGE=ubuntu:16.04 SWIFT_SNAPSHOT=$SWIFT_DEVELOPMENT_SNAPSHOT + # Test GCD_ASYNCH codepath on Linux + - os: linux + dist: trusty + sudo: required + services: docker + env: DOCKER_IMAGE=ubuntu:16.04 SWIFT_SNAPSHOT=$SWIFT_DEVELOPMENT_SNAPSHOT CUSTOM_TEST_SCRIPT=testWithGCD.sh DOCKER_ENVIRONMENT=CUSTOM_TEST_SCRIPT - os: linux dist: trusty sudo: required diff --git a/Sources/KituraNet/IncomingSocketManager.swift b/Sources/KituraNet/IncomingSocketManager.swift index eca5600..bc6059d 100644 --- a/Sources/KituraNet/IncomingSocketManager.swift +++ b/Sources/KituraNet/IncomingSocketManager.swift @@ -50,7 +50,16 @@ In particular, it is in charge of: public class IncomingSocketManager { /// A mapping from socket file descriptor to IncomingSocketHandler - var socketHandlers = [Int32: IncomingSocketHandler]() + private var socketHandlers = [Int32: IncomingSocketHandler]() + private var shQueue = DispatchQueue(label: "socketHandlers.queue", attributes: .concurrent) + + internal var socketHandlerCount: Int { + var result: Int = 0 + shQueue.sync { + result = socketHandlers.count + } + return result + } /// Interval at which to check for idle sockets to close let keepAliveIdleCheckingInterval: TimeInterval = 5.0 @@ -151,7 +160,9 @@ public class IncomingSocketManager { try socket.setBlocking(mode: false) let handler = IncomingSocketHandler(socket: socket, using: processor) - socketHandlers[socket.socketfd] = handler + shQueue.sync(flags: .barrier) { + socketHandlers[socket.socketfd] = handler + } #if !GCD_ASYNCH && os(Linux) var event = epoll_event() @@ -205,21 +216,22 @@ public class IncomingSocketManager { Log.error("Error occurred on a file descriptor of an epool wait") } else { - if let handler = socketHandlers[event.data.fd] { - - if (event.events & EPOLLOUT.rawValue) != 0 { - handler.handleWrite() - } - if (event.events & EPOLLIN.rawValue) != 0 { - let processed = handler.handleRead() - if !processed { - deferredHandlingNeeded = true - deferredHandlers[event.data.fd] = handler + shQueue.sync { + if let handler = socketHandlers[event.data.fd] { + if (event.events & EPOLLOUT.rawValue) != 0 { + handler.handleWrite() + } + if (event.events & EPOLLIN.rawValue) != 0 { + let processed = handler.handleRead() + if !processed { + deferredHandlingNeeded = true + deferredHandlers[event.data.fd] = handler + } } } - } - else { - Log.error("No handler for file descriptor \(event.data.fd)") + else { + Log.error("No handler for file descriptor \(event.data.fd)") + } } } } @@ -258,38 +270,32 @@ public class IncomingSocketManager { /// 2. Removing the reference to the IncomingHTTPSocketHandler /// 3. Have the IncomingHTTPSocketHandler close the socket /// - /// **Note:** In order to safely update the socketHandlers Dictionary the removal - /// of idle sockets is done in the thread that is accepting new incoming sockets - /// after a socket was accepted. Had this been done in a timer, there would be a - /// to have a lock around the access to the socketHandlers Dictionary. The other - /// idea here is that if sockets aren't coming in, it doesn't matter too much if - /// we leave a round some idle sockets. - /// /// - Parameter allSockets: flag indicating if the manager is shutting down, and we should cleanup all sockets, not just idle ones private func removeIdleSockets(removeAll: Bool = false) { let now = Date() guard removeAll || now.timeIntervalSince(keepAliveIdleLastTimeChecked) > keepAliveIdleCheckingInterval else { return } - - let maxInterval = now.timeIntervalSinceReferenceDate - for (fileDescriptor, handler) in socketHandlers { - if !removeAll && handler.processor != nil && (handler.processor?.inProgress ?? false || maxInterval < handler.processor?.keepAliveUntil ?? maxInterval) { - continue - } - socketHandlers.removeValue(forKey: fileDescriptor) + shQueue.sync(flags: .barrier) { + let maxInterval = now.timeIntervalSinceReferenceDate + for (fileDescriptor, handler) in socketHandlers { + if !removeAll && handler.processor != nil && (handler.processor?.inProgress ?? false || maxInterval < handler.processor?.keepAliveUntil ?? maxInterval) { + continue + } + socketHandlers.removeValue(forKey: fileDescriptor) - #if !GCD_ASYNCH && os(Linux) - let result = epoll_ctl(epollDescriptor(fd: fileDescriptor), EPOLL_CTL_DEL, fileDescriptor, nil) - if result == -1 { - if errno != EBADF && // Ignore EBADF error (bad file descriptor), probably got closed. - errno != ENOENT { // Ignore ENOENT error (No such file or directory), probably got closed. - Log.error("epoll_ctl failure. Error code=\(errno). Reason=\(lastError())") + #if !GCD_ASYNCH && os(Linux) + let result = epoll_ctl(epollDescriptor(fd: fileDescriptor), EPOLL_CTL_DEL, fileDescriptor, nil) + if result == -1 { + if errno != EBADF && // Ignore EBADF error (bad file descriptor), probably got closed. + errno != ENOENT { // Ignore ENOENT error (No such file or directory), probably got closed. + Log.error("epoll_ctl failure. Error code=\(errno). Reason=\(lastError())") + } } - } - #endif - - handler.prepareToClose() + #endif + + handler.prepareToClose() + } + keepAliveIdleLastTimeChecked = Date() } - keepAliveIdleLastTimeChecked = Date() } /// Private method to return the last error based on the value of errno. diff --git a/Tests/KituraNetTests/SocketManagerTests.swift b/Tests/KituraNetTests/SocketManagerTests.swift index ddfb8ff..8b9b563 100644 --- a/Tests/KituraNetTests/SocketManagerTests.swift +++ b/Tests/KituraNetTests/SocketManagerTests.swift @@ -53,7 +53,7 @@ class SocketManagerTests: KituraNetTest { let socket1 = try Socket.create() let processor1 = TestIncomingSocketProcessor() manager.handle(socket: socket1, processor: processor1) - XCTAssertEqual(manager.socketHandlers.count, 1, "There should be 1 IncomingSocketHandler, there are \(manager.socketHandlers.count)") + XCTAssertEqual(manager.socketHandlerCount, 1, "There should be 1 IncomingSocketHandler, there are \(manager.socketHandlerCount)") // The check for idle sockets to clean up happens when new sockets arrive. // However the check is done at most once a minute. To avoid waiting a minute @@ -64,7 +64,7 @@ class SocketManagerTests: KituraNetTest { let socket2 = try Socket.create() let processor2 = TestIncomingSocketProcessor() manager.handle(socket: socket2, processor: processor2) - XCTAssertEqual(manager.socketHandlers.count, 2, "There should be 2 IncomingSocketHandler, there are \(manager.socketHandlers.count)") + XCTAssertEqual(manager.socketHandlerCount, 2, "There should be 2 IncomingSocketHandler, there are \(manager.socketHandlerCount)") // Enable cleanup the next time there is a "new incoming socket" (see description above) manager.keepAliveIdleLastTimeChecked = Date().addingTimeInterval(-120.0) @@ -75,7 +75,7 @@ class SocketManagerTests: KituraNetTest { let socket3 = try Socket.create() let processor3 = TestIncomingSocketProcessor() manager.handle(socket: socket3, processor: processor3) - XCTAssertEqual(manager.socketHandlers.count, 3, "There should be 3 IncomingSocketHandler, there are \(manager.socketHandlers.count)") + XCTAssertEqual(manager.socketHandlerCount, 3, "There should be 3 IncomingSocketHandler, there are \(manager.socketHandlerCount)") // Enable cleanup the next time there is a "new incoming socket" (see description above) manager.keepAliveIdleLastTimeChecked = Date().addingTimeInterval(-120.0) @@ -85,7 +85,7 @@ class SocketManagerTests: KituraNetTest { let socket4 = try Socket.create() let processor4 = TestIncomingSocketProcessor() manager.handle(socket: socket4, processor: processor4) - XCTAssertEqual(manager.socketHandlers.count, 3, "There should be 3 IncomingSocketHandler, there are \(manager.socketHandlers.count)") + XCTAssertEqual(manager.socketHandlerCount, 3, "There should be 3 IncomingSocketHandler, there are \(manager.socketHandlerCount)") } catch let error { XCTFail("Failed to create a socket. Error=\(error)") diff --git a/testWithGCD.sh b/testWithGCD.sh new file mode 100755 index 0000000..47437e9 --- /dev/null +++ b/testWithGCD.sh @@ -0,0 +1,2 @@ +#!/bin/bash +swift test -Xswiftc -DGCD_ASYNCH