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

adopt sendable #218

Merged
merged 4 commits into from
Jul 1, 2022
Merged
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
10 changes: 9 additions & 1 deletion 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: _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.
Expand Down Expand Up @@ -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
24 changes: 21 additions & 3 deletions Sources/Logging/Logging.swift
Original file line number Diff line number Diff line change
Expand Up @@ -721,8 +721,11 @@ extension Logger {
case string(String)

/// A metadata value which is some `CustomStringConvertible`.
#if compiler(>=5.6)
case stringConvertible(CustomStringConvertible & Sendable)
Copy link
Member

@fabianfett fabianfett Apr 12, 2022

Choose a reason for hiding this comment

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

Why does the value need to be Sendable here? Also this might be breaking as is.

I think to make this non breaking we need:

@preconcurrency protocol LogMetadataCustomStringConvertible: CustomStringConvertible & Sendable {}

And then we can go for:

case stringConvertible(LogMetadataCustomStringConvertible)

Copy link
Contributor

Choose a reason for hiding this comment

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

Because otherwise MetadataValue cannot conform to Sendable, which in term means Logger.Metadata cannot conform to Sendable, which in the end means Logger can't conform to Sendable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think to make this non breaking we need:

@preconcurrency protocol LogMetadataCustomStringConvertible: CustomStringConvertible & Sendable {}

This wouldn't work either, because this would be a new protocol. Currently, it only requires CustomStringConvertible. Maybe a typealias would work:

@preconcurrency typealias LogMetadataCustomStringConvertible = CustomStringConvertible & Sendable

But I'm not sure if @preconcurrency is supported in this case.

Copy link
Member

@fabianfett fabianfett Apr 12, 2022

Choose a reason for hiding this comment

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

Because otherwise MetadataValue cannot conform to Sendable, which in term means Logger.Metadata cannot conform to Sendable, which in the end means Logger can't conform to Sendable.

Yeah, I get that. However we could also transform CustomStringConvertible to a string directly on the call-site. This would allow us to not require Sendable. However a class that might change its string representation at a later point, would always be represented by its original value, if it is added to the metadata.

This wouldn't work either, because this would be a new protocol. Currently, it only requires CustomStringConvertible.

@ffried Very good point. Going directly to string might potentially allow us to work around the semver major here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is more to that... stringConvertible is currently kind of an escape hatch to pass arbitrary objects to the log handlers. We use it to for example control which file a message is logged to in CocoaLumberjack's log handler. Most of these usages are probably fine with adding Sendable, though.

Also, changing the enum case this way will also result in a semver major.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ktoso are you saying that adding the Sendable protocol requirement will only yield warnings until Swift 6?

Copy link
Member

Choose a reason for hiding this comment

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

That's right, I just confirmed with Doug as well. If you were seeing an error in beta-1 (maybe?), then Doug says this could have been a bug and is still just a warning nowadays and will continue to be.

Copy link
Member Author

Choose a reason for hiding this comment

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

in that case I am also comfortable with this soft break.

@ffried @fabianfett @weissi @Lukasa wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. A warning is fine. 👍
It's the same as when for example a new deprecation is added. There would also be a warning then.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with a warning here.

#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
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand All @@ -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
}
Expand Down Expand Up @@ -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
14 changes: 13 additions & 1 deletion Tests/LoggingTests/LoggingTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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?

Expand Down Expand Up @@ -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
29 changes: 15 additions & 14 deletions Tests/LoggingTests/TestLogger.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}
}

Expand All @@ -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
}
}
}
Expand Down Expand Up @@ -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
43 changes: 43 additions & 0 deletions Tests/LoggingTests/TestSendable.swift
Original file line number Diff line number Diff line change
@@ -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
}
2 changes: 1 addition & 1 deletion docker/docker-compose.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,4 @@ services:

shell:
<<: *common
entrypoint: /bin/bash
entrypoint: /bin/bash -l
2 changes: 1 addition & 1 deletion scripts/soundness.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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... "
Expand Down