Skip to content

Commit

Permalink
Swift 5: resolve Linux crash with concurrent access to socketHandlers (…
Browse files Browse the repository at this point in the history 
…#284)
  • Loading branch information
@djones6
djones6 authored Jan 8, 2019
1 parent 23895b6 commit c88223a
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 44 deletions.
6 changes: 6 additions & 0 deletions .travis.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
86 changes: 46 additions & 40 deletions Sources/KituraNet/IncomingSocketManager.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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)")
}
}
}
}
Expand Down Expand Up @@ -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.
Expand Down
8 changes: 4 additions & 4 deletions Tests/KituraNetTests/SocketManagerTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)")
Expand Down
2 changes: 2 additions & 0 deletions testWithGCD.sh
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
#!/bin/bash
swift test -Xswiftc -DGCD_ASYNCH

0 comments on commit c88223a

Please sign in to comment.