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

RUM-1053 Make Logger Sendable for Swift 6 concurrency #2035

Merged
merged 8 commits into from
Sep 10, 2024

Conversation

ncreated
Copy link
Member

@ncreated ncreated commented Sep 3, 2024

What and why?

📦 This PR prepares DatadogLogs for compatibility with apps that enable "complete" concurrency checking in Swift 6.

I identified a potential data-race safety issue when passing instances of LoggerProtocol across isolation boundaries, such as to different threads:

import DatadogLogs

let logger = Logger.create(with: .init())

DispatchQueue.global(qos: .userInitiated).async {
    logger.debug("foo") 
    // ^ ❌ Error: Capture of 'logger' with non-sendable type 'any LoggerProtocol' in a `@Sendable` closure
}

How?

This issue is resolved by declaring Sendable conformance for LoggerProtocol:

A conformance to Sendable means the given type is thread safe, and values of the type can be shared across arbitrary isolation domains without introducing a risk of data races.

Since Xcode 15 uses SWIFT_STRICT_CONCURRENCY: minimal, which checks only explicit Sendable conformances, this:

- public protocol LoggerProtocol {
+ public protocol LoggerProtocol: Sendable {

resulted in concurrency warnings elsewhere.

Instead of using @unchecked Sendable for RemoteLogger, ConsoleLogger, and some test types, I chose to address concurrency warnings properly, making RemoteLogger and ConsoleLogger fully checked Sendable types 👌.

This required several major changes:

  • Followed RUM-3462 refactor: Make FeatureScope always available #1744 and replaced the mutable core reference in RemoteLogger with an immutable reference to FeatureScope.
  • Made FeatureScope sendable, to facilitate similar work in other modules.
  • Reworked how global Logs attributes are passed to RemoteLogger after removing the core reference.
  • Introduced SynchronizedAttributes and SynchronizedTags to ensure RemoteLogger has only Sendable members.
  • Added test coverage for Logger tags, which was previously missing.

Smaller adjustments were made as a consequence of spreading Sendable conformance. For example, marking BacktraceReporter as Sendable required resolving issues in Crash Reporting tests, as this type is shared through DatadogInternal.

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference
  • Add CHANGELOG entry for user facing changes

@ncreated ncreated self-assigned this Sep 3, 2024
@ncreated ncreated force-pushed the ncreated/RUM-1053/make-logs-api-concurrency-safe branch from 614a84c to 87c4a0a Compare September 4, 2024 15:54
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Sep 4, 2024

Datadog Report

Branch report: ncreated/RUM-1053/make-logs-api-concurrency-safe
Commit report: a3d8423
Test service: dd-sdk-ios

✅ 0 Failed, 1880 Passed, 0 Skipped, 1m 16.57s Total Time
🔻 Test Sessions change in coverage: 2 decreased, 5 increased, 1 no change

🔻 Code Coverage Decreases vs Default Branch (2)

  • test DatadogRUMTests iOS 81.15% (-0.02%) - Details
  • test DatadogCoreTests iOS 72.74% (-0.01%) - Details

@ncreated ncreated force-pushed the ncreated/RUM-1053/make-logs-api-concurrency-safe branch from 87c4a0a to a3d8423 Compare September 5, 2024 08:33
@ncreated ncreated changed the title RUM-1053 Make DatadogLogs public API concurrency-safe RUM-1053 Make Logger Sendable for Swift 6 concurrency Sep 5, 2024
Comment on lines -26 to -33
} else {
let iso8601Formatter = DateFormatter()
iso8601Formatter.locale = Locale(identifier: "en_US_POSIX")
iso8601Formatter.timeZone = TimeZone(abbreviation: "UTC")! // swiftlint:disable:this force_unwrapping
iso8601Formatter.calendar = Calendar(identifier: .gregorian)
iso8601Formatter.dateFormat = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'" // ISO8601 format
return iso8601Formatter
}
Copy link
Member Author

Choose a reason for hiding this comment

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

💡 not needed anymore after we dropped iOS 11

@ncreated ncreated marked this pull request as ready for review September 5, 2024 09:28
@ncreated ncreated requested review from a team as code owners September 5, 2024 09:28
/// need to be safely managed across multiple threads or tasks.
internal final class SynchronizedAttributes: Sendable {
/// The underlying dictionary of attributes, wrapped in a `ReadWriteLock` to ensure thread safety.
private let attributes: ReadWriteLock<[String: Encodable]>
Copy link
Member

Choose a reason for hiding this comment

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

/question We are not using the ReadWriteLock as a property wrapper in multiple places, is it because it would require it to be a var? Can't this be solved by making ReadWriteLock Sendable?

Copy link
Member Author

Choose a reason for hiding this comment

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

The ReadWriteLock is Sendable already - it was done in this PR. And yes, if using property wrapper, this will become var, raising concurrency issue. I agree with what was discussed here - property wrappers become quite unusable with strict concurrency.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, makes sense 👍

Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ncreated ncreated merged commit 76fdab4 into develop Sep 10, 2024
16 checks passed
@ncreated ncreated deleted the ncreated/RUM-1053/make-logs-api-concurrency-safe branch September 10, 2024 06:39
@mariedm mariedm mentioned this pull request Sep 11, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants