Merge pull request #399 from Vkt0r/threading-issue

Fix a crash when several request for the same URL or route are launched together

Author: Vkt0r

.circleci/config.yml

@@ -1,7 +1,8 @@
 version: 2
 
 jobs:
-  danger:
+
+  Danger:
     macos:
       xcode: 10.2.0
     steps:
@@ -24,7 +25,8 @@ jobs:
       - run:
           name: Danger
           command: bundle exec danger
-  macos:
+
+  Test OS X Platform:
     environment:
       TEST_REPORTS: /tmp/test-results
       LANG: en_US.UTF-8
@@ -58,14 +60,11 @@ jobs:
       - store_artifacts:
           path: /tmp/test-results
 
-  linux:
+  Test Linux Platform:
     docker:
-      - image: swift:4.2
+      - image: swift:5.0
     steps:
       - checkout
-      - run: 
-          name: Compile code
-          command: swift build
       - run: 
           name: Run Unit Tests
           command: swift test
@@ -74,10 +73,10 @@ workflows:
   version: 2
   tests:
     jobs:
-      - danger
-      - linux:
+      - Danger
+      - Test Linux Platform:
           requires:
-            - danger
-      - macos:
+            - Danger
+      - Test OS X Platform:
           requires:
-            - danger
+            - Danger

.swiftlint.yml

@@ -17,5 +17,6 @@ disabled_rules:
 
 excluded: # paths to ignore during linting. Takes precedence over `included`.
   - LinuxMain.swift
+  - XCode/Tests/XCTestManifests.swift
   - Tests/XCTestManifests.swift
   - Package.swift

CHANGELOG.md

@@ -23,6 +23,7 @@ All notable changes to this project will be documented in this file. Changes not
 - Added [Danger](https://danger.systems/ruby/) and Swiftlint to the project. ([#398](https://github.com/httpswift/swifter/pull/398)) by [@Vkt0r](https://github.com/Vkt0r)
 
 ## Fixed
+- An issue causing a crash regarding a thread race condition. ([#399](https://github.com/httpswift/swifter/pull/399)) by [@Vkt0r](https://github.com/Vkt0r)
 - An issue in the `HttpRouter` causing issues to handle routes with overlapping in the tail. ([#379](https://github.com/httpswift/swifter/pull/359), [#382](https://github.com/httpswift/swifter/pull/382)) by [@Vkt0r](https://github.com/Vkt0r)
 
 - Fixes build errors by excluding XC(UI)Test files from regular targets [#397](https://github.com/httpswift/swifter/pull/397)) by [@ChristianSteffens](https://github.com/ChristianSteffens)

Package.swift

@@ -1,4 +1,4 @@
-// swift-tools-version:4.0
+// swift-tools-version:5.0
 
 import PackageDescription
 
@@ -13,8 +13,25 @@ let package = Package(
   dependencies: [],
 
   targets: [
-    .target(name: "Swifter", dependencies: [], path: "XCode/Sources"),
-    .target(name: "Example", dependencies: ["Swifter"], path: "Example"),
-    .testTarget(name: "SwifterTests", dependencies: ["Swifter"], path: "XCode/Tests")
+    .target(
+      name: "Swifter", 
+      dependencies: [], 
+      path: "XCode/Sources"
+      ),
+
+    .target(
+      name: "Example", 
+      dependencies: [
+        "Swifter"
+      ], 
+      path: "Example"),
+
+    .testTarget(
+      name: "SwifterTests", 
+      dependencies: [
+        "Swifter"
+      ], 
+      path: "XCode/Tests"
+    )
   ]
 )

XCode/Sources/HttpRouter.swift

@@ -24,6 +24,9 @@ open class HttpRouter {
     }
 
     private var rootNode = Node()
+    
+    /// The Queue to handle the thread safe access to the routes
+    private let queue = DispatchQueue(label: "swifter.httpserverio.httprouter")
 
     public func routes() -> [String] {
         var routes = [String]()
@@ -56,21 +59,26 @@ open class HttpRouter {
     }
 
     public func route(_ method: String?, path: String) -> ([String: String], (HttpRequest) -> HttpResponse)? {
-        if let method = method {
-            let pathSegments = (method + "/" + stripQuery(path)).split("/")
+        
+        return queue.sync {
+            if let method = method {
+                let pathSegments = (method + "/" + stripQuery(path)).split("/")
+                var pathSegmentsGenerator = pathSegments.makeIterator()
+                var params = [String: String]()
+                if let handler = findHandler(&rootNode, params: &params, generator: &pathSegmentsGenerator) {
+                    return (params, handler)
+                }
+            }
+            
+            let pathSegments = ("*/" + stripQuery(path)).split("/")
             var pathSegmentsGenerator = pathSegments.makeIterator()
             var params = [String: String]()
             if let handler = findHandler(&rootNode, params: &params, generator: &pathSegmentsGenerator) {
                 return (params, handler)
             }
+            
+            return nil
         }
-        let pathSegments = ("*/" + stripQuery(path)).split("/")
-        var pathSegmentsGenerator = pathSegments.makeIterator()
-        var params = [String: String]()
-        if let handler = findHandler(&rootNode, params: &params, generator: &pathSegmentsGenerator) {
-            return (params, handler)
-        }
-        return nil
     }
 
     private func inflate(_ node: inout Node, generator: inout IndexingIterator<[String]>) -> Node {

XCode/Sources/HttpServerIO.swift

@@ -85,7 +85,9 @@ public class HttpServerIO {
                     strongSelf.queue.async {
                         strongSelf.sockets.insert(socket)
                     }
+                    
                     strongSelf.handleConnection(socket)
+                    
                     strongSelf.queue.async {
                         strongSelf.sockets.remove(socket)
                     }

XCode/Swifter.xcodeproj/project.pbxproj

@@ -45,7 +45,6 @@
 		269B47981D3AAAE20042D137 /* Errno.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7C76B2A11D369C9D00D35BFB /* Errno.swift */; };
 		269B47991D3AAAE20042D137 /* String+BASE64.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7C76B6F61D2C44F30030FC98 /* String+BASE64.swift */; };
 		269B47A71D3AAC4F0042D137 /* SwiftertvOS.h in Headers */ = {isa = PBXBuildFile; fileRef = 269B47A51D3AAC4F0042D137 /* SwiftertvOS.h */; settings = {ATTRIBUTES = (Public, ); }; };
-		542604AE226A33540065B874 /* XCTestManifests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7B912F49220507D00062C360 /* XCTestManifests.swift */; };
 		6A0D4512204E9988000A0726 /* MimeTypesTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6A0D4511204E9988000A0726 /* MimeTypesTests.swift */; };
 		6AE2FF722048013000302EC4 /* MimeTypes.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6AE2FF702048011A00302EC4 /* MimeTypes.swift */; };
 		6AE2FF732048013000302EC4 /* MimeTypes.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6AE2FF702048011A00302EC4 /* MimeTypes.swift */; };
@@ -55,6 +54,9 @@
 		7AE893FE1C0512C400A29F63 /* SwifterMac.h in Headers */ = {isa = PBXBuildFile; fileRef = 7AE893FD1C0512C400A29F63 /* SwifterMac.h */; settings = {ATTRIBUTES = (Public, ); }; };
 		7AE8940D1C05151100A29F63 /* Launch Screen.storyboard in Resources */ = {isa = PBXBuildFile; fileRef = 7AE8940C1C05151100A29F63 /* Launch Screen.storyboard */; };
 		7B11AD4B21C9A8A6002F8820 /* SwifterTestsHttpRouter.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7CCB8C5F1D97B8CC008B9712 /* SwifterTestsHttpRouter.swift */; };
+		7B55EC95226E0E4F00042D23 /* ServerThreadingTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7B55EC94226E0E4F00042D23 /* ServerThreadingTests.swift */; };
+		7B55EC96226E0E4F00042D23 /* ServerThreadingTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7B55EC94226E0E4F00042D23 /* ServerThreadingTests.swift */; };
+		7B55EC97226E0E4F00042D23 /* ServerThreadingTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7B55EC94226E0E4F00042D23 /* ServerThreadingTests.swift */; };
 		7B74CFA82163C40F001BE07B /* AppDelegate.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7CDAB80C1BE2A1D400C8A977 /* AppDelegate.swift */; };
 		7C377E181D964B96009C6148 /* String+File.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7C377E161D964B6A009C6148 /* String+File.swift */; };
 		7C377E191D964B9F009C6148 /* String+File.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7C377E161D964B6A009C6148 /* String+File.swift */; };
@@ -177,6 +179,7 @@
 		7AE893FD1C0512C400A29F63 /* SwifterMac.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = SwifterMac.h; sourceTree = "<group>"; };
 		7AE893FF1C0512C400A29F63 /* Info.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = Info.plist; sourceTree = "<group>"; };
 		7AE8940C1C05151100A29F63 /* Launch Screen.storyboard */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = file.storyboard; path = "Launch Screen.storyboard"; sourceTree = "<group>"; };
+		7B55EC94226E0E4F00042D23 /* ServerThreadingTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ServerThreadingTests.swift; sourceTree = "<group>"; };
 		7B912F49220507D00062C360 /* XCTestManifests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = XCTestManifests.swift; sourceTree = "<group>"; };
 		7B912F4B220507DB0062C360 /* LinuxMain.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = LinuxMain.swift; sourceTree = "<group>"; };
 		7C377E161D964B6A009C6148 /* String+File.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "String+File.swift"; sourceTree = "<group>"; };
@@ -417,6 +420,7 @@
 				7C4785E81C71D15600A9FE73 /* SwifterTestsWebSocketSession.swift */,
 				0858E7F31D68BB2600491CD1 /* IOSafetyTests.swift */,
 				6A0D4511204E9988000A0726 /* MimeTypesTests.swift */,
+				7B55EC94226E0E4F00042D23 /* ServerThreadingTests.swift */,
 			);
 			path = Tests;
 			sourceTree = "<group>";
@@ -769,6 +773,7 @@
 				047F1F02226AB9AD00909B95 /* XCTestManifests.swift in Sources */,
 				043660CE21FED35500497989 /* SwifterTestsHttpParser.swift in Sources */,
 				043660D521FED36C00497989 /* MimeTypesTests.swift in Sources */,
+				7B55EC96226E0E4F00042D23 /* ServerThreadingTests.swift in Sources */,
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 		};
@@ -781,6 +786,7 @@
 				043660EA21FED51E00497989 /* IOSafetyTests.swift in Sources */,
 				043660E821FED51900497989 /* SwifterTestsStringExtensions.swift in Sources */,
 				043660E521FED51100497989 /* SwifterTestsHttpRouter.swift in Sources */,
+				7B55EC97226E0E4F00042D23 /* ServerThreadingTests.swift in Sources */,
 				043660E621FED51400497989 /* SwifterTestsHttpParser.swift in Sources */,
 				043660EB21FED52000497989 /* MimeTypesTests.swift in Sources */,
 			);
@@ -896,6 +902,7 @@
 				043660D421FED36900497989 /* IOSafetyTests.swift in Sources */,
 				7CCD87721C660B250068099B /* SwifterTestsStringExtensions.swift in Sources */,
 				7B11AD4B21C9A8A6002F8820 /* SwifterTestsHttpRouter.swift in Sources */,
+				7B55EC95226E0E4F00042D23 /* ServerThreadingTests.swift in Sources */,
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 		};
@@ -1278,7 +1285,7 @@
 				OTHER_SWIFT_FLAGS = "-Xfrontend -warn-long-function-bodies=500";
 				SDKROOT = macosx;
 				SWIFT_OPTIMIZATION_LEVEL = "-Onone";
-				SWIFT_VERSION = 4.2;
+				SWIFT_VERSION = 5.0;
 				TARGETED_DEVICE_FAMILY = "1,2";
 			};
 			name = Debug;
@@ -1333,7 +1340,7 @@
 				OTHER_SWIFT_FLAGS = "-Xfrontend -warn-long-function-bodies=500";
 				SDKROOT = macosx;
 				SWIFT_OPTIMIZATION_LEVEL = "-Owholemodule";
-				SWIFT_VERSION = 4.2;
+				SWIFT_VERSION = 5.0;
 				TARGETED_DEVICE_FAMILY = "1,2";
 				VALIDATE_PRODUCT = YES;
 			};

XCode/Tests/IOSafetyTests.swift

@@ -11,16 +11,22 @@ import XCTest
 
 class IOSafetyTests: XCTestCase {
     var server: HttpServer!
+    var urlSession: URLSession!
 
     override func setUp() {
         super.setUp()
         server = HttpServer.pingServer()
+        urlSession = URLSession(configuration: .default)
     }
 
     override func tearDown() {
         if server.operating {
             server.stop()
         }
+        
+        urlSession = nil
+        server = nil
+        
         super.tearDown()
     }
 
@@ -29,10 +35,10 @@ class IOSafetyTests: XCTestCase {
             server = HttpServer.pingServer()
             do {
                 try server.start()
-                XCTAssertFalse(URLSession.shared.retryPing())
+                XCTAssertFalse(urlSession.retryPing())
                 (0...100).forEach { _ in
                     DispatchQueue.global(qos: DispatchQoS.default.qosClass).sync {
-                        URLSession.shared.pingTask { _, _, _ in }.resume()
+                        urlSession.pingTask { _, _, _ in }.resume()
                     }
                 }
                 server.stop()

XCode/Tests/ServerThreadingTests.swift

@@ -0,0 +1,115 @@
+//
+//  ServerThreadingTests.swift
+//  Swifter
+//
+//  Created by Victor Sigler on 4/22/19.
+//  Copyright © 2019 Damian Kołakowski. All rights reserved.
+//
+
+import XCTest
+@testable import Swifter
+
+class ServerThreadingTests: XCTestCase {
+    
+    var server: HttpServer!
+    
+    override func setUp() {
+        super.setUp()
+        server = HttpServer()
+    }
+    
+    override func tearDown() {
+        if server.operating {
+            server.stop()
+        }
+        server = nil
+        super.tearDown()
+    }
+    
+    func testShouldHandleTheSameRequestWithDifferentTimeIntervals() {
+        
+        let path = "/a/:b/c"
+        let queue = DispatchQueue(label: "com.swifter.threading")
+        let hostURL: URL
+        
+        server.GET[path] = { .ok(.html("You asked for " + $0.path)) }
+        
+        do {
+            
+            #if os(Linux)
+            try server.start(9081)
+            hostURL = URL(string: "http://localhost:9081")!
+            #else
+            try server.start()
+            hostURL = defaultLocalhost
+            #endif
+            
+            let requestExpectation = expectation(description: "Request should finish.")
+            requestExpectation.expectedFulfillmentCount = 3
+            
+            (1...3).forEach { index in
+                queue.asyncAfter(deadline: .now() + .seconds(index)) {
+                    let task = URLSession.shared.executeAsyncTask(hostURL: hostURL, path: path) { (_, response, _ ) in
+                        requestExpectation.fulfill()
+                        let statusCode = (response as? HTTPURLResponse)?.statusCode
+                        XCTAssertNotNil(statusCode)
+                        XCTAssertEqual(statusCode, 200, "\(hostURL)")
+                    }
+                    
+                    task.resume()
+                }
+            }
+            
+        } catch let error {
+            XCTFail("\(error)")
+        }
+        
+        waitForExpectations(timeout: 10, handler: nil)
+    }
+    
+    func testShouldHandleTheSameRequestConcurrently() {
+        
+        let path = "/a/:b/c"
+        server.GET[path] = { .ok(.html("You asked for " + $0.path)) }
+        
+        var requestExpectation: XCTestExpectation? = expectation(description: "Should handle the request concurrently")
+        
+        do {
+            
+            try server.start()
+            let downloadGroup = DispatchGroup()
+            
+            DispatchQueue.concurrentPerform(iterations: 3) { _ in
+                downloadGroup.enter()
+                
+                let task = URLSession.shared.executeAsyncTask(path: path) { (_, response, _ ) in
+                    
+                    let statusCode = (response as? HTTPURLResponse)?.statusCode
+                    XCTAssertNotNil(statusCode)
+                    XCTAssertEqual(statusCode, 200)
+                    requestExpectation?.fulfill()
+                    requestExpectation = nil
+                    downloadGroup.leave()
+                }
+                
+                task.resume()
+            }
+            
+        } catch let error {
+            XCTFail("\(error)")
+        }
+        
+        waitForExpectations(timeout: 15, handler: nil)
+    }
+}
+
+extension URLSession {
+    
+    func executeAsyncTask(
+        hostURL: URL = defaultLocalhost,
+        path: String,
+        completionHandler handler: @escaping (Data?, URLResponse?, Error?) -> Void
+        ) -> URLSessionDataTask {
+        return self.dataTask(with: hostURL.appendingPathComponent(path), completionHandler: handler)
+    }
+}