From dfca57046ce26b8a919c4d5dd73851c55fe96ed3 Mon Sep 17 00:00:00 2001 From: Nathan Harris Date: Mon, 23 Sep 2019 19:36:07 -0700 Subject: [PATCH 1/2] Run thread sanitizer on 5.1 test CI jobs Motivation: Now that SwiftPM with 5.1 supports Linux thread sanitizers, we should run this as part of the normal test pass to catch threading issues. Modifications: Add `--sanitize=thread` argument to `swift test` commands for any job that runs Swift >=5.1. Result: More bugs should be caught as soon as possible now that thread races are being monitored. --- .gitlab-ci.yml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index d244854..2a204ec 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -67,17 +67,20 @@ build 5.1ubuntu-bionic: variables: REDIS_URL: 'redis' REDIS_PW: 'password' + SANITIZER_ARG: '--sanitize=thread' services: - name: redis:5 alias: 'redis' command: ["redis-server", "--requirepass", "password"] script: - swift build -v - - swift test + - swift test $SANITIZER_ARG test 5.0:ubuntu-xenial: extends: .test image: swift:5.0-xenial + variables: + SANITIZER_ARG: '' test 5.1:ubuntu-xenial: extends: .test image: swift:5.1-xenial @@ -89,6 +92,8 @@ test latest:ubuntu-xenial: test 5.0:ubuntu-bionic: extends: .test image: swift:5.0-bionic + variables: + SANITIZER_ARG: '' test 5.1:ubuntu-bionic: extends: .test image: swift:5.1-bionic From 939e41b832a5f9b2a69add509aed9d76ef6b532e Mon Sep 17 00:00:00 2001 From: Nathan Harris Date: Mon, 23 Sep 2019 19:57:48 -0700 Subject: [PATCH 2/2] Fix data race with `RedisMetrics.activeConnectionCount` Motivation: After enabling the thread sanitizer during testing, a data race with the `activeConnectionCount` in `RedisMetrics` was caught due to changing a primitive `Int` across threads. Modifications: Add a specialized class `ActiveConnectionGauge` to wrap the semantics of the `activeConnectionCount` with an `Atomic` property. Result: No data races should occur with the `RedisMetrics` subsystem. --- Sources/RediStack/RedisConnection.swift | 6 ++--- Sources/RediStack/RedisMetrics.swift | 36 +++++++++++++++++++------ 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/Sources/RediStack/RedisConnection.swift b/Sources/RediStack/RedisConnection.swift index a37cc75..6c3616c 100644 --- a/Sources/RediStack/RedisConnection.swift +++ b/Sources/RediStack/RedisConnection.swift @@ -148,7 +148,7 @@ public final class RedisConnection: RedisClient { self.logger[metadataKey: loggingKeyID] = "\(UUID())" self._state = .open self.logger.debug("Connection created.") - RedisMetrics.activeConnectionCount += 1 + RedisMetrics.activeConnectionCount.increment() RedisMetrics.totalConnectionCount.increment() // attach a callback to the channel to capture situations where the channel might be closed out from under @@ -160,7 +160,7 @@ public final class RedisConnection: RedisClient { self.state = .closed self.logger.warning("Channel was closed unexpectedly.") - RedisMetrics.activeConnectionCount -= 1 + RedisMetrics.activeConnectionCount.decrement() } } @@ -243,7 +243,7 @@ extension RedisConnection { notification.whenSuccess { self.state = .closed self.logger.debug("Connection closed.") - RedisMetrics.activeConnectionCount -= 1 + RedisMetrics.activeConnectionCount.decrement() } return notification diff --git a/Sources/RediStack/RedisMetrics.swift b/Sources/RediStack/RedisMetrics.swift index 7934279..d8b386c 100644 --- a/Sources/RediStack/RedisMetrics.swift +++ b/Sources/RediStack/RedisMetrics.swift @@ -13,6 +13,7 @@ //===----------------------------------------------------------------------===// import Metrics +import NIOConcurrencyHelpers /// The system funnel for all `Metrics` interactions from the Redis library. /// @@ -38,14 +39,8 @@ public struct RedisMetrics { } } - private static let activeConnectionCountGauge = Gauge(label: .activeConnectionCount) - /// The current number of connections this library has active. - /// - Note: Changing this number will update the `Metrics.Gauge` stored for recording the new value. - public static var activeConnectionCount: Int = 0 { - didSet { - activeConnectionCountGauge.record(activeConnectionCount) - } - } + /// The wrapped `Metrics.Gauge` maintaining the current number of connections this library has active. + public static var activeConnectionCount = ActiveConnectionGauge() /// The `Metrics.Counter` that retains the number of connections made since application startup. public static let totalConnectionCount = Counter(label: .totalConnectionCount) /// The `Metrics.Counter` that retains the number of commands that successfully returned from Redis @@ -61,6 +56,31 @@ public struct RedisMetrics { private init() { } } +/// A specialized wrapper class for working with `Metrics.Gauge` objects for the purpose of an incrementing or decrementing count of active Redis connections. +public class ActiveConnectionGauge { + private let gauge = Gauge(label: .activeConnectionCount) + private let count = Atomic(value: 0) + + /// The number of the connections that are currently reported as active. + var currentCount: Int { return count.load() } + + internal init() { } + + /// Increments the current count by the amount specified. + /// - Parameter amount: The number to increase the current count by. Default is `1`. + public func increment(by amount: Int = 1) { + _ = self.count.add(amount) + self.gauge.record(self.count.load()) + } + + /// Decrements the current count by the amount specified. + /// - Parameter amount: The number to decrease the current count by. Default is `1`. + public func decrement(by amount: Int = 1) { + _ = self.count.sub(amount) + self.gauge.record(self.count.load()) + } +} + extension Metrics.Counter { @inline(__always) convenience init(label: RedisMetrics.Label) {