Issue #1818: Serialize access of a rule-based formatter#1889
Issue #1818: Serialize access of a rule-based formatter#1889itingliu wants to merge 1 commit intoswiftlang:mainfrom
Conversation
ICU rule-based number formatter isn't thread safe. Add a lock to serialize access to this formatter if we happen to use one. This issue is fixed in [upstream ICU](unicode-org/icu#3928), so we will be able to remove this once the fix is avaiable.
|
@swift-ci please test |
itingliu
changed the title
|
|
||
| /// `Sendable` notes: `UNumberFormat` is safe to use from multple threads after initialization and configuration. | ||
| // `Sendable` notes: `UNumberFormat` backed by ICU's `DecimalFormat` is safe to use from multiple threads after initialization and configuration. However, `RuleBasedNumberFormat`is not thread-safe. See https://github.com/unicode-org/icu/pull/3928 | ||
| let uformatter: UnsafeMutablePointer<UNumberFormat?> |
There was a problem hiding this comment.
Part of me wonders if it's worth wrapping this in a sub type that requires you to access it through withLockIfNeeded like Mutex would. Maybe it's too much ceremony for what it's worth. What do you think?
There was a problem hiding this comment.
Do you mean a
class AlwaysSafeFormatter {
private let uformatter: UnsafeMutablePointer<UNumberFormat?>
private let formatLock: Mutex<Void>?
internal func withLock(...) ... { }
}
And we use AlwaysSafeFormatter.withLock everywhere?
I can see that, but ICULegacyNumberFormatter is already a wrapper. It's also the only (hopefully) place where we optionally lock around an ICU object, so I think for now it's ok to leave it here. But if we ever encounter this pattern again (hopefully not) then I agree we could benefit from this wrapper
There was a problem hiding this comment.
Yeah something like that, just to avoid accidentally forgetting to use the lock. Sounds good yeah I figured it might be a bit more architecture than it's worth if it's all isolated to a small space
2 participants
Resolves #1818
ICU rule-based number formatter isn't thread safe. Add a lock to serialize access to this formatter if we happen to use one.
This issue is fixed in upstream ICU, so we will be able to remove this once the fix is available.