Skip to content

Make the Logger thread-safe to avoid crashes #2775

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Sep 15, 2023
Merged

Make the Logger thread-safe to avoid crashes #2775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5f6ad8b
Make Logger thread-safe to avoid crashes
nuno-vieira Sep 13, 2023
543a3b2
Update CHANGELOG.md
nuno-vieira Sep 13, 2023
f9012a8
Make the logger queue concurrent
nuno-vieira Sep 14, 2023
aa9936e
Forgot to make the LogCongig's queue concurrent
nuno-vieira Sep 15, 2023
5c982ab
Merge branch 'develop' into fix/logger-not-thread-safe
nuno-vieira Sep 15, 2023
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
### 🐞 Fixed
- Fix core data warnings when logging with different user [#2759](https://github.com/GetStream/stream-chat-swift/pull/2759)
- Fix connecting user from background thread [#2762](https://github.com/GetStream/stream-chat-swift/pull/2762)
- Make the Logger thread-safe to avoid crashes [#2775](https://github.com/GetStream/stream-chat-swift/pull/2775)
- Improve `addDevice()` and `removeDevice()` with optimistic updates [#2778](https://github.com/GetStream/stream-chat-swift/pull/2778)

## StreamChatUI
Expand Down
68 changes: 47 additions & 21 deletions Sources/StreamChat/Utils/Logger/Logger.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,20 +178,26 @@ public enum LogConfig {
/// Underlying logger instance to control singleton.
private static var _logger: Logger?

private static var queue = DispatchQueue(label: "io.getstream.logconfig", attributes: .concurrent)

/// Logger instance to be used by StreamChat.
///
/// - Important: Other options in `LogConfig` will not take affect if this is changed.
public static var logger: Logger {
get {
if let logger = _logger {
return logger
} else {
_logger = Logger(identifier: identifier, destinations: destinations)
return _logger!
queue.sync {
if let logger = _logger {
return logger
} else {
_logger = Logger(identifier: identifier, destinations: destinations)
return _logger!
}
}
}
set {
_logger = newValue
queue.async(flags: .barrier) {
_logger = newValue
}
}
}

Expand All @@ -209,14 +215,27 @@ public class Logger {

/// Destinations for this logger.
/// See `LogDestination` protocol for details.
public var destinations: [LogDestination]
public var destinations: [LogDestination] {
get {
loggerQueue.sync {
_destinations
}
}
set {
loggerQueue.async(flags: .barrier) { [weak self] in
self?._destinations = newValue
}
}
}

private var _destinations: [LogDestination]

private let loggerQueue = DispatchQueue(label: "LoggerQueue \(UUID())")
private let loggerQueue = DispatchQueue(label: "io.getstream.logger", attributes: .concurrent)

/// Init a logger with a given identifier and destinations.
public init(identifier: String = "", destinations: [LogDestination] = []) {
self.identifier = identifier
self.destinations = destinations
_destinations = destinations
}

/// Allows logger to be called as function.
Expand Down Expand Up @@ -266,18 +285,25 @@ public class Logger {
let enabledDestinations = destinations.filter { $0.isEnabled(level: level, subsystems: subsystems) }
guard !enabledDestinations.isEmpty else { return }

let logDetails = LogDetails(
loggerIdentifier: identifier,
level: level,
date: Date(),
message: String(describing: message()),
threadName: threadName,
functionName: functionName,
fileName: fileName,
lineNumber: lineNumber
)
for destination in enabledDestinations {
loggerQueue.async {
// The message() closure should be done from the thread it was called.
// In some scenarios message() will print out managedObjectContexts and in this case
// it is important the closure is performed in the managedObjectContext's thread.
let messageString = String(describing: message())

loggerQueue.async(flags: .barrier) { [weak self] in
guard let self = self else { return }

let logDetails = LogDetails(
loggerIdentifier: self.identifier,
level: level,
date: Date(),
message: messageString,
threadName: self.threadName,
functionName: functionName,
fileName: fileName,
lineNumber: lineNumber
)
for destination in enabledDestinations {
destination.process(logDetails: logDetails)
}
}
Expand Down
4 changes: 4 additions & 0 deletions StreamChat.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1196,6 +1196,7 @@
AD552E0228F46CE700199A6F /* ImageLoaderOptions.swift in Sources */ = {isa = PBXBuildFile; fileRef = AD552E0028F46CE700199A6F /* ImageLoaderOptions.swift */; };
AD57979E2978C4F7006CC435 /* UploadedAttachmentPostProcessor.swift in Sources */ = {isa = PBXBuildFile; fileRef = AD57979D2978C4F7006CC435 /* UploadedAttachmentPostProcessor.swift */; };
AD57979F2978C4F7006CC435 /* UploadedAttachmentPostProcessor.swift in Sources */ = {isa = PBXBuildFile; fileRef = AD57979D2978C4F7006CC435 /* UploadedAttachmentPostProcessor.swift */; };
AD5BCCC92AB22A6600456CD9 /* Logger_Tests.swift in Sources */ = {isa = PBXBuildFile; fileRef = AD5BCCC82AB22A6600456CD9 /* Logger_Tests.swift */; };
AD61F3D92A27A9FB00247B5D /* ChannelMemberUnbanRequestPayload_Tests.swift in Sources */ = {isa = PBXBuildFile; fileRef = AD61F3D82A27A9FB00247B5D /* ChannelMemberUnbanRequestPayload_Tests.swift */; };
AD6A248A280DA890003BA1E4 /* PushDevice.swift in Sources */ = {isa = PBXBuildFile; fileRef = AD6A2489280DA88F003BA1E4 /* PushDevice.swift */; };
AD6A248B280DA890003BA1E4 /* PushDevice.swift in Sources */ = {isa = PBXBuildFile; fileRef = AD6A2489280DA88F003BA1E4 /* PushDevice.swift */; };
Expand Down Expand Up @@ -3633,6 +3634,7 @@
AD552E0028F46CE700199A6F /* ImageLoaderOptions.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ImageLoaderOptions.swift; sourceTree = "<group>"; };
AD57979D2978C4F7006CC435 /* UploadedAttachmentPostProcessor.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = UploadedAttachmentPostProcessor.swift; sourceTree = "<group>"; };
AD57DE752A77D5A2005408B6 /* ChannelListSearchStrategy_Tests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ChannelListSearchStrategy_Tests.swift; sourceTree = "<group>"; };
AD5BCCC82AB22A6600456CD9 /* Logger_Tests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Logger_Tests.swift; sourceTree = "<group>"; };
AD61F3D82A27A9FB00247B5D /* ChannelMemberUnbanRequestPayload_Tests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ChannelMemberUnbanRequestPayload_Tests.swift; sourceTree = "<group>"; };
AD6A2489280DA88F003BA1E4 /* PushDevice.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PushDevice.swift; sourceTree = "<group>"; };
AD6BEFEF2786070800E184B4 /* SwitchButton.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SwitchButton.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -6623,6 +6625,7 @@
79CD959324F9381700E87377 /* MulticastDelegate_Tests.swift */,
C163ECFD2992B06C006D6124 /* NotificationExtensionLifecycle_Tests.swift */,
C14A46552845064E00EF498E /* ThreadSafeWeakCollection_Tests.swift */,
AD5BCCC82AB22A6600456CD9 /* Logger_Tests.swift */,
ADF617662A09925300E70307 /* MessagesPaginationStateHandling */,
A364D0BD27D12C600029857A /* Database */,
A3F65E3B27EB7357003F6256 /* EquatableEvent */,
Expand Down Expand Up @@ -10567,6 +10570,7 @@
79B5517C24E6A1CA00CE9FEC /* MessagePayloads_Tests.swift in Sources */,
F6D61D9D2510B57F00EB0624 /* NSManagedObject_Tests.swift in Sources */,
79D6CE9525F7D72E00BE2EEC /* ChatChannelWatcherListController_Tests.swift in Sources */,
AD5BCCC92AB22A6600456CD9 /* Logger_Tests.swift in Sources */,
A34ECB5027F5CAF200A804C1 /* PinnedMessagesQuery_IntegrationTests.swift in Sources */,
79896D66250A6D1800BA8F1C /* ChannelReadUpdaterMiddleware_Tests.swift in Sources */,
AC82033C28C61F640002EFDD /* CreateCallPayload_Tests.swift in Sources */,
Expand Down
31 changes: 31 additions & 0 deletions Tests/StreamChatTests/Utils/Logger_Tests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
//
// Copyright © 2023 Stream.io Inc. All rights reserved.
//

import Foundation

import Foundation
@testable import StreamChat
import XCTest

final class Logger_Tests: XCTestCase {
func test_log_isThreadSafe() {
LogConfig.destinationTypes = [ConsoleLogDestination.self]

DispatchQueue.concurrentPerform(iterations: 100) { _ in
Copy link
Contributor

@ipavlidakis ipavlidakis Sep 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Without your changes, are those tests failing?

Copy link
Member Author

@nuno-vieira nuno-vieira Sep 15, 2023

Choose a reason for hiding this comment

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

Yes 👍

log.error("should not crash")
}

DispatchQueue.concurrentPerform(iterations: 100) { _ in
log.warning("should not crash")
}

DispatchQueue.concurrentPerform(iterations: 100) { _ in
log.debug("should not crash")
}

DispatchQueue.concurrentPerform(iterations: 100) { _ in
log.info("should not crash")
}
}
}