Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce Sendable conformance for LogHandlers Swift-Log, and make Logger Sendable #217

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion Package.swift
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// swift-tools-version:5.0
// swift-tools-version:5.5
//===----------------------------------------------------------------------===//
//
// This source file is part of the Swift Logging API open source project
Expand All @@ -17,6 +17,10 @@ import PackageDescription

let package = Package(
name: "swift-log",
platforms: [
.iOS(.v13),
.macOS(.v10_15),
],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't just do that like this; we need to support old platforms too

products: [
.library(name: "Logging", targets: ["Logging"]),
],
Expand Down
4 changes: 2 additions & 2 deletions Sources/Logging/LogHandler.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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: Sendable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't just do it like this, we'd need to follow the trickery with the sendable availability as explained in https://github.com/swift-server/guides/blob/main/docs/concurrency-adoption-guidelines.md#backwards-compatibility-of-declarations-and-checked-swift-concurrency

So we'd need _SwiftLog_Sendable and the entire dance

/// 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.
Expand All @@ -126,7 +126,7 @@ public protocol LogHandler {
/// - file: The file the log message was emitted from.
/// - function: The function the log line was emitted from.
/// - line: The line the log message was emitted from.
func log(level: Logger.Level,
@Sendable func log(level: Logger.Level,
message: Logger.Message,
metadata: Logger.Metadata?,
source: String,
Expand Down
16 changes: 9 additions & 7 deletions Sources/Logging/Logging.swift
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import WASILibc
///
/// logger.info("Hello World!")
///
public struct Logger {
public struct Logger: Sendable {
@usableFromInline
var handler: LogHandler

Expand Down Expand Up @@ -707,15 +707,15 @@ extension Logger {
/// over `..., metadata: ["colors": .array([.string("\(user.topColor)"), .string("\(user.secondColor)")])`
/// - prefer `logger.info("nested info", metadata: ["nested": ["fave-numbers": ["\(1)", "\(2)", "\(3)"], "foo": "bar"]])`
/// over `..., metadata: ["nested": .dictionary(["fave-numbers": ...])])`
public enum MetadataValue {
public enum MetadataValue: Sendable {
/// A metadata value which is a `String`.
///
/// Because `MetadataValue` implements `ExpressibleByStringInterpolation`, and `ExpressibleByStringLiteral`,
/// you don't need to type `.string(someType.description)` you can use the string interpolation `"\(someType)"`.
case string(String)

/// A metadata value which is some `CustomStringConvertible`.
case stringConvertible(CustomStringConvertible)
case stringConvertible(CustomStringConvertible & Sendable)

/// A metadata value which is a dictionary from `String` to `Logger.MetadataValue`.
///
Expand All @@ -734,7 +734,7 @@ extension Logger {
///
/// Log levels are ordered by their severity, with `.trace` being the least severe and
/// `.critical` being the most severe.
public enum Level: String, Codable, CaseIterable {
public enum Level: String, Codable, CaseIterable, Sendable {
/// Appropriate for messages that contain information normally of use only when
/// tracing the execution of a program.
case trace
Expand Down Expand Up @@ -999,7 +999,7 @@ internal typealias CFilePointer = UnsafeMutablePointer<FILE>
/// A wrapper to facilitate `print`-ing to stderr and stdio that
/// ensures access to the underlying `FILE` is locked to prevent
/// cross-thread interleaving of output.
internal struct StdioOutputStream: TextOutputStream {
internal struct StdioOutputStream: TextOutputStream, Sendable {
internal let file: CFilePointer
internal let flushMode: FlushMode

Expand Down Expand Up @@ -1074,6 +1074,8 @@ 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 {
public typealias Stream = TextOutputStream & Sendable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

has to use the compatibility trickery


/// 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)
Expand All @@ -1084,7 +1086,7 @@ public struct StreamLogHandler: LogHandler {
return StreamLogHandler(label: label, stream: StdioOutputStream.stderr)
}

private let stream: TextOutputStream
private let stream: Stream
private let label: String

public var logLevel: Logger.Level = .info
Expand All @@ -1106,7 +1108,7 @@ public struct StreamLogHandler: LogHandler {
}

// internal for testing only
internal init(label: String, stream: TextOutputStream) {
internal init(label: String, stream: Stream) {
self.label = label
self.stream = stream
}
Expand Down
33 changes: 20 additions & 13 deletions Tests/LoggingTests/LoggingTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -262,24 +262,28 @@ 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 {
private var makeValue: (() -> String)?
internal class LazyMetadataBox: CustomStringConvertible, @unchecked Sendable {
typealias ValueMaker = @Sendable () -> String
let lock = NSLock()
private var makeValue: ValueMaker?
private var _value: String?

public init(_ makeValue: @escaping () -> String) {
public init(_ makeValue: @escaping ValueMaker) {
self.makeValue = makeValue
}

/// This allows caching a value in case it is accessed via an by name subscript,
// rather than as part of rendering all metadata that a LoggingContext was carrying
public var value: String {
if let f = self.makeValue {
self._value = f()
self.makeValue = nil
}
self.lock.withLock {
if let f = self.makeValue {
self._value = f()
self.makeValue = nil
}

assert(self._value != nil, "_value MUST NOT be nil once `lazyValue` has run.")
return self._value!
assert(self._value != nil, "_value MUST NOT be nil once `lazyValue` has run.")
return self._value!
}
}

public var description: String {
Expand Down Expand Up @@ -672,14 +676,17 @@ class LoggingTest: XCTestCase {
XCTAssertLessThan(Logger.Level.error, Logger.Level.critical)
}

final class InterceptStream: TextOutputStream {
final class InterceptStream: TextOutputStream, @unchecked Sendable {
let lock = NSLock()
var interceptedText: String?
var strings = [String]()

func write(_ string: String) {
// This is a test implementation, a real implementation would include locking
self.strings.append(string)
self.interceptedText = (self.interceptedText ?? "") + string
lock.withLock {
// This is a test implementation, a real implementation would include locking
self.strings.append(string)
self.interceptedText = (self.interceptedText ?? "") + string
}
}
}

Expand Down
6 changes: 3 additions & 3 deletions Tests/LoggingTests/TestLogger.swift
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ internal struct TestLogging {
var history: History { return self.recorder }
}

internal struct TestLogHandler: LogHandler {
internal struct TestLogHandler: LogHandler, @unchecked Sendable {
private let logLevelLock = NSLock()
private let metadataLock = NSLock()
private let recorder: Recorder
Expand Down Expand Up @@ -96,7 +96,7 @@ internal struct TestLogHandler: LogHandler {
}
}

internal class Config {
internal class Config: @unchecked Sendable {
private static let ALL = "*"

private let lock = NSLock()
Expand All @@ -122,7 +122,7 @@ internal class Config {
}
}

internal class Recorder: History {
internal class Recorder: History, @unchecked Sendable {
private let lock = NSLock()
private var _entries = [LogEntry]()

Expand Down