Repository navigation
iOS,macOS: make Logger thread-safe, conform to Sendable - #190488
Merged
Merged
Conversation
github-actions
Bot
added
platform-ios
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces thread safety to the Darwin platform Logger by conforming LogLevel, Logger, and OutputWriter types to Sendable (or @unchecked Sendable) and protecting shared state with an NSLock. It also adds a concurrent test to verify behavior under TSan. Feedback on the changes highlights compatibility issues with older iOS and macOS deployment targets due to the use of lock.withLock (which requires iOS 16.0+ / macOS 13.0+), recommending manual lock() and unlock() calls instead. Additionally, the review points out a potential data race in StringOutputWriter where mutable properties are accessed concurrently in tests without synchronization.
cbracken
force-pushed
the
logger-threadsafe
branch
2 times, most recently
from
August 4, 2026 02:53
118a193 to
16c52a0
Compare
Previously, `Logger.shared` was a mutable static variable, and updating `Logger.logLevel` or `Logger.outputWriter` replaced the entire singleton instance without any synchronisation. This could cause data races when logging from multiple threads, and prevented `Logger` from conforming to `Sendable` under strict Swift concurrency. We now make `shared` an immutable constant and guard its mutable `logLevel` and `outputWriter` state behind a lock. The caller-supplied message autoclosure is evaluated outside the lock, both to keep the lock scope minimal and to avoid deadlocking should the closure re-enter `Logger` since the lock is not reentrant. Issue: flutter#44030
cbracken
force-pushed
the
logger-threadsafe
branch
from
August 4, 2026 06:56
16c52a0 to
9bb1ba1
Compare
LongCatIsLooong
approved these changes
Aug 4, 2026
| // someone accidentally calling the Logger from within the autoclosure. | ||
| let line = message() | ||
| lock.withLock { | ||
| _outputWriter.writeLine(level: level, line) |
Contributor
There was a problem hiding this comment.
It looks like writeLine calls are always protected by a NSLock in Logger, but the OutputWriter.writeLine implementation typically still have be thread-safe because it can be used outside of Logger.
I feel it makes a little bit more sense to make it OutputWriter's responsibility to make sure its writeLine implementation is thread-safe (which is already the case since OutputWriter must be Sendable and writeLine is nonisolated), instead of trying to guarantee thread-safety on both ends, so:
lock.withLock { _outputWriter }.writeLine(level: level, message())A potential issue with the current implementation is that in case the output writer does something weird in writeLine (like accessing the Logger) it looks possible to cause a deadlock since the lock is not recursive.
Member
Author
There was a problem hiding this comment.
In our case OutputWriter is our own internal protocol and both implementations are at the bottom of this file and don't use locks so we're safe. I've got a followup that replaces this altogether with os_log which will solve it altogether though!
This was referenced Aug 5, 2026
auto-submit Bot
pushed a commit
to flutter/packages
that referenced
this pull request
Aug 10, 2026
…12420) Manual roll requested by [email protected] flutter/flutter@b766512...27b0988 2026-08-05 [email protected] reland(tool): remove redundant --enable-experiment=record-use flag (flutter/flutter#190591) 2026-08-05 [email protected] Windows: Propagate enabled accessibility state (flutter/flutter#190507) 2026-08-05 [email protected] Roll Fuchsia Test Scripts from ltbuIH9Z3T_yOuigu... to vcANVO8VIDQHasH1X... (flutter/flutter#190589) 2026-08-05 [email protected] Document frozen embedder API structs (flutter/flutter#186842) 2026-08-05 [email protected] Document super call order for State.didChangeDependencies (flutter/flutter#185945) 2026-08-05 [email protected] Move examples of `flutter/widgets` widgets out from `flutter/material` (flutter/flutter#189532) 2026-08-05 [email protected] [web] Use thread local strike caches in skwasm (flutter/flutter#190048) 2026-08-05 [email protected] doc: fix typo in see also section for PrimaryScrollController.maybeOf (flutter/flutter#190386) 2026-08-05 [email protected] Migrate the shell unit tests from legacy Dart native functions to FFI (flutter/flutter#190473) 2026-08-05 [email protected] render proxy box now defaults baseline calculation to null (flutter/flutter#190269) 2026-08-05 [email protected] Update Widgets Localizations from Translation Console (flutter/flutter#190503) 2026-08-04 [email protected] Revert: fix(tool): remove redundant --enable-experiment=record-use flag (flutter/flutter#190583) 2026-08-04 [email protected] Roll Fuchsia Test Scripts from 1frGe_KltAJKkeyPg... to ltbuIH9Z3T_yOuigu... (flutter/flutter#190561) 2026-08-04 [email protected] iOS,macOS: add tsan and ubsan support for Swift (flutter/flutter#190497) 2026-08-04 [email protected] fix: update on_message_ to nullptr after window destroy so that dart gets destroy message (flutter/flutter#185807) 2026-08-04 [email protected] [flutter_tools] Gracefully handle locked Windows files during clean (flutter/flutter#190095) 2026-08-04 [email protected] iOS,macOS: make Logger thread-safe, conform to Sendable (flutter/flutter#190488) 2026-08-04 [email protected] iOS: Eliminate use of IOSContextNoop in platform view tests (reland) (flutter/flutter#190509) 2026-08-04 [email protected] fix(tool): remove redundant --enable-experiment=record-use flag (flutter/flutter#190475) 2026-08-04 [email protected] android_hardware_smoke_test: Detect blank image failures or EGL initialization warnings and retry (flutter/flutter#190110) 2026-08-04 [email protected] Bumps text gamma on windows to match skia. (flutter/flutter#190477) 2026-08-04 [email protected] Roll Skia from 48b58ee222f1 to a8583a0a2c11 (2 revisions) (flutter/flutter#190537) 2026-08-04 [email protected] [tool][web] Intercept dart2wasm errors & append JS migration footers (flutter/flutter#190476) 2026-08-04 [email protected] [record_use] Migrate IconTreeShaker to `package:record_use` (flutter/flutter#190225) 2026-08-04 [email protected] Handle unexpected exceptions during Azure metadata detection (flutter/flutter#189457) 2026-08-04 [email protected] Fix merge conflict from flutter/flutter#190369 (flutter/flutter#190544) 2026-08-04 [email protected] Prepare device support symbols (flutter/flutter#190369) 2026-08-04 [email protected] Roll Packages from ac87e65 to 3498b9d (1 revision) (flutter/flutter#190532) 2026-08-04 [email protected] Roll Skia from a08d918ebd6a to 48b58ee222f1 (11 revisions) (flutter/flutter#190527) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously,
Logger.sharedwas a mutable static variable, and updatingLogger.logLevelorLogger.outputWriterreplaced the entire singleton instance without any synchronisation. This could cause data races when logging from multiple threads, and preventedLoggerfrom conforming toSendableunder strict Swift concurrency.We now make
sharedan immutable constant and guard its mutablelogLevelandoutputWriterstate behind a lock.The caller-supplied message autoclosure is evaluated outside the lock, both to keep the lock scope minimal and to avoid deadlocking should the closure re-enter
Loggersince the lock is not reentrant.Issue: #44030
Issue: #181683
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.