From f2e8667d508016531ba5d044b739eae1ae532a30 Mon Sep 17 00:00:00 2001 From: tomer doron Date: Fri, 1 Jul 2022 16:38:19 -0700 Subject: [PATCH] adopt sendable (#218) motivation: adjust to swift 5.6 changes: * define sendable shims for protocols and structs that may be used in async context * adjust tests * add a test to make sure no warning are emitted --- Sources/Logging/LogHandler.swift | 10 ++++++- Sources/Logging/Logging.swift | 24 +++++++++++++-- Tests/LoggingTests/LoggingTest.swift | 14 ++++++++- Tests/LoggingTests/TestLogger.swift | 29 +++++++++--------- Tests/LoggingTests/TestSendable.swift | 43 +++++++++++++++++++++++++++ docker/docker-compose.yaml | 2 +- scripts/soundness.sh | 2 +- 7 files changed, 103 insertions(+), 21 deletions(-) create mode 100644 Tests/LoggingTests/TestSendable.swift diff --git a/Sources/Logging/LogHandler.swift b/Sources/Logging/LogHandler.swift index 6b52b120..48fac76f 100644 --- a/Sources/Logging/LogHandler.swift +++ b/Sources/Logging/LogHandler.swift @@ -113,7 +113,7 @@ /// Please note that the above `LogHandler` will still pass the 'log level is a value' test above it iff the global log /// level has not been overridden. And most importantly it passes the requirement listed above: A change to the log /// level on one `Logger` should not affect the log level of another `Logger` variable. -public protocol LogHandler { +public protocol LogHandler: _SwiftLogSendableLogHandler { /// This method is called when a `LogHandler` must emit a log message. There is no need for the `LogHandler` to /// check if the `level` is above or below the configured `logLevel` as `Logger` already performed this check and /// determined that a message should be logged. @@ -186,3 +186,11 @@ extension LogHandler { line: line) } } + +// MARK: - Sendable support helpers + +#if compiler(>=5.6) +@preconcurrency public protocol _SwiftLogSendableLogHandler: Sendable {} +#else +public protocol _SwiftLogSendableLogHandler {} +#endif diff --git a/Sources/Logging/Logging.swift b/Sources/Logging/Logging.swift index ab49d21c..106c5bc9 100644 --- a/Sources/Logging/Logging.swift +++ b/Sources/Logging/Logging.swift @@ -721,8 +721,11 @@ extension Logger { case string(String) /// A metadata value which is some `CustomStringConvertible`. + #if compiler(>=5.6) + case stringConvertible(CustomStringConvertible & Sendable) + #else case stringConvertible(CustomStringConvertible) - + #endif /// A metadata value which is a dictionary from `String` to `Logger.MetadataValue`. /// /// Because `MetadataValue` implements `ExpressibleByDictionaryLiteral`, you don't need to type @@ -1076,6 +1079,12 @@ let systemStdout = WASILibc.stdout! /// `StreamLogHandler` is a simple implementation of `LogHandler` for directing /// `Logger` output to either `stderr` or `stdout` via the factory methods. public struct StreamLogHandler: LogHandler { + #if compiler(>=5.6) + internal typealias _SendableTextOutputStream = TextOutputStream & Sendable + #else + internal typealias _SendableTextOutputStream = TextOutputStream + #endif + /// Factory that makes a `StreamLogHandler` to directs its output to `stdout` public static func standardOutput(label: String) -> StreamLogHandler { return StreamLogHandler(label: label, stream: StdioOutputStream.stdout) @@ -1086,7 +1095,7 @@ public struct StreamLogHandler: LogHandler { return StreamLogHandler(label: label, stream: StdioOutputStream.stderr) } - private let stream: TextOutputStream + private let stream: _SendableTextOutputStream private let label: String public var logLevel: Logger.Level = .info @@ -1108,7 +1117,7 @@ public struct StreamLogHandler: LogHandler { } // internal for testing only - internal init(label: String, stream: TextOutputStream) { + internal init(label: String, stream: _SendableTextOutputStream) { self.label = label self.stream = stream } @@ -1264,3 +1273,12 @@ extension Logger.MetadataValue: ExpressibleByArrayLiteral { self = .array(elements) } } + +// MARK: - Sendable support helpers + +#if compiler(>=5.6) +extension Logger: Sendable {} +extension Logger.MetadataValue: Sendable {} +extension Logger.Level: Sendable {} +extension Logger.Message: Sendable {} +#endif diff --git a/Tests/LoggingTests/LoggingTest.swift b/Tests/LoggingTests/LoggingTest.swift index 3400c8ef..441b6f94 100644 --- a/Tests/LoggingTests/LoggingTest.swift +++ b/Tests/LoggingTests/LoggingTest.swift @@ -262,7 +262,7 @@ class LoggingTest: XCTestCase { // Example of custom "box" which may be used to implement "render at most once" semantics // Not thread-safe, thus should not be shared across threads. - internal class LazyMetadataBox: CustomStringConvertible { + internal final class LazyMetadataBox: CustomStringConvertible { private var makeValue: (() -> String)? private var _value: String? @@ -889,3 +889,15 @@ extension Logger { } #endif } + +// Sendable + +#if compiler(>=5.6) +// used to test logging metadata which requires Sendable conformance +// @unchecked Sendable since manages it own state +extension LoggingTest.LazyMetadataBox: @unchecked Sendable {} + +// used to test logging stream which requires Sendable conformance +// @unchecked Sendable since manages it own state +extension LoggingTest.InterceptStream: @unchecked Sendable {} +#endif diff --git a/Tests/LoggingTests/TestLogger.swift b/Tests/LoggingTests/TestLogger.swift index 984649c1..2e880ac9 100644 --- a/Tests/LoggingTests/TestLogger.swift +++ b/Tests/LoggingTests/TestLogger.swift @@ -31,8 +31,6 @@ internal struct TestLogging { } internal struct TestLogHandler: LogHandler { - private let logLevelLock = NSLock() - private let metadataLock = NSLock() private let recorder: Recorder private let config: Config private var logger: Logger // the actual logger @@ -56,10 +54,10 @@ internal struct TestLogHandler: LogHandler { var logLevel: Logger.Level { get { // get from config unless set - return self.logLevelLock.withLock { self._logLevel } ?? self.config.get(key: self.label) + return self._logLevel ?? self.config.get(key: self.label) } set { - self.logLevelLock.withLock { self._logLevel = newValue } + self._logLevel = newValue } } @@ -72,26 +70,20 @@ internal struct TestLogHandler: LogHandler { public var metadata: Logger.Metadata { get { - // return self.logger.metadata - return self.metadataLock.withLock { self._metadata } + return self._metadata } set { - // self.logger.metadata = newValue - self.metadataLock.withLock { self._metadata = newValue } + self._metadata = newValue } } // TODO: would be nice to delegate to local copy of logger but StdoutLogger is a reference type. why? subscript(metadataKey metadataKey: Logger.Metadata.Key) -> Logger.Metadata.Value? { get { - // return self.logger[metadataKey: metadataKey] - return self.metadataLock.withLock { self._metadata[metadataKey] } + return self._metadata[metadataKey] } set { - // return logger[metadataKey: metadataKey] = newValue - self.metadataLock.withLock { - self._metadata[metadataKey] = newValue - } + self._metadata[metadataKey] = newValue } } } @@ -342,3 +334,12 @@ internal struct TestLibrary { } } } + +// Sendable + +#if compiler(>=5.6) +extension TestLogHandler: @unchecked Sendable {} +extension Recorder: @unchecked Sendable {} +extension Config: @unchecked Sendable {} +extension MDC: @unchecked Sendable {} +#endif diff --git a/Tests/LoggingTests/TestSendable.swift b/Tests/LoggingTests/TestSendable.swift new file mode 100644 index 00000000..9e96e7b7 --- /dev/null +++ b/Tests/LoggingTests/TestSendable.swift @@ -0,0 +1,43 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift Logging API open source project +// +// Copyright (c) 2022 Apple Inc. and the Swift Logging API project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of Swift Logging API project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// +import Dispatch +@testable import Logging +import XCTest + +class SendableTest: XCTestCase { + #if compiler(>=5.6) + func testSendableLogger() async { + let testLogging = TestLogging() + LoggingSystem.bootstrapInternal(testLogging.make) + + let logger = Logger(label: "test") + let message1 = Logger.Message(stringLiteral: "critical 1") + let message2 = Logger.Message(stringLiteral: "critical 2") + let metadata: Logger.Metadata = ["key": "value"] + + let task = Task.detached { + logger.info("info") + logger.critical(message1) + logger.critical(message2) + logger.warning(.init(stringLiteral: "warning"), metadata: metadata) + } + + await task.value + testLogging.history.assertExist(level: .info, message: "info", metadata: [:]) + testLogging.history.assertExist(level: .critical, message: "critical 1", metadata: [:]) + testLogging.history.assertExist(level: .critical, message: "critical 2", metadata: [:]) + testLogging.history.assertExist(level: .warning, message: "warning", metadata: metadata) + } + #endif +} diff --git a/docker/docker-compose.yaml b/docker/docker-compose.yaml index cb5a8470..69ab5c73 100644 --- a/docker/docker-compose.yaml +++ b/docker/docker-compose.yaml @@ -34,4 +34,4 @@ services: shell: <<: *common - entrypoint: /bin/bash + entrypoint: /bin/bash -l diff --git a/scripts/soundness.sh b/scripts/soundness.sh index 64fb7d14..f2a00dc4 100755 --- a/scripts/soundness.sh +++ b/scripts/soundness.sh @@ -32,7 +32,7 @@ here="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" function replace_acceptable_years() { # this needs to replace all acceptable forms with 'YEARS' - sed -e 's/20[12][78901]-20[12][8901]/YEARS/' -e 's/20[12][8901]/YEARS/' + sed -e 's/20[12][789012]-20[12][89012]/YEARS/' -e 's/20[12][89012]/YEARS/' } printf "=> Checking linux tests... "