Skip to content

Commit

Permalink
Merge pull request httpswift#399 from Vkt0r/threading-issue
Browse files Browse the repository at this point in the history
Fix a crash when several request for the same URL or route are launched together
  • Loading branch information
Vkt0r authored May 1, 2019
2 parents 5734a82 + 3165a19 commit a5dfa25
Show file tree
Hide file tree
Showing 11 changed files with 248 additions and 59 deletions.
23 changes: 11 additions & 12 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
version: 2

jobs:
danger:

Danger:
macos:
xcode: 10.2.0
steps:
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
1 change: 1 addition & 0 deletions .swiftlint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
25 changes: 21 additions & 4 deletions Package.swift
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// swift-tools-version:4.0
// swift-tools-version:5.0

import PackageDescription

Expand All @@ -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"
)
]
)
26 changes: 17 additions & 9 deletions XCode/Sources/HttpRouter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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]()
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 2 additions & 0 deletions XCode/Sources/HttpServerIO.swift
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,9 @@ public class HttpServerIO {
strongSelf.queue.async {
strongSelf.sockets.insert(socket)
}

strongSelf.handleConnection(socket)

strongSelf.queue.async {
strongSelf.sockets.remove(socket)
}
Expand Down
13 changes: 10 additions & 3 deletions XCode/Swifter.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -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 */; };
Expand All @@ -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 */; };
Expand Down Expand Up @@ -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>"; };
Expand Down Expand Up @@ -417,6 +420,7 @@
7C4785E81C71D15600A9FE73 /* SwifterTestsWebSocketSession.swift */,
0858E7F31D68BB2600491CD1 /* IOSafetyTests.swift */,
6A0D4511204E9988000A0726 /* MimeTypesTests.swift */,
7B55EC94226E0E4F00042D23 /* ServerThreadingTests.swift */,
);
path = Tests;
sourceTree = "<group>";
Expand Down Expand Up @@ -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;
};
Expand All @@ -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 */,
);
Expand Down Expand Up @@ -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;
};
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
};
Expand Down
10 changes: 8 additions & 2 deletions XCode/Tests/IOSafetyTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}

Expand All @@ -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()
Expand Down
115 changes: 115 additions & 0 deletions XCode/Tests/ServerThreadingTests.swift
Original file line number Diff line number Diff line change
@@ -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)
}
}
Loading

0 comments on commit a5dfa25

Please sign in to comment.