Skip to content

Issue #1818: Serialize access of a rule-based formatter #1889

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

Open
itingliu wants to merge 1 commit into
base: main
Choose a base branch
from
+85 −45
Open

Issue #1818: Serialize access of a rule-based formatter #1889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
110 changes: 65 additions & 45 deletions Sources/FoundationInternationalization/Formatting/Number/ICULegacyNumberFormatter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,27 +15,39 @@ import FoundationEssentials
#endif

internal import _FoundationICU
internal import Synchronization

@available(macOS 12.0, iOS 15.0, tvOS 15.0, watchOS 8.0, *)
internal final class ICULegacyNumberFormatter : @unchecked Sendable {

/// `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?>
Copy link
Copy Markdown
Contributor

@jmschonfeld jmschonfeld Apr 10, 2026

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

@itingliu itingliu Apr 10, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@jmschonfeld jmschonfeld Apr 10, 2026

Choose a reason for hiding this comment

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

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

// Optional lock to serializes access to uformatter.
// FIXME: Remove the lock once ICU fix is available
private let formatLock: Mutex<Void>?

private init(openedFormatter: UnsafeMutablePointer<UNumberFormat?>) {
private init(openedFormatter: UnsafeMutablePointer<UNumberFormat?>, needsLock: Bool) {
uformatter = openedFormatter
formatLock = needsLock ? Mutex(()) : nil
}

private func withLockIfNeeded<T>(_ body: () -> T) -> T {
formatLock?.withLock { _ in body() } ?? body()
}

deinit {
// We don't need to lock around since the race in ICU only happens in formatting and parsing.
unum_close(uformatter)
}

func parseAsInt(_ string: some StringProtocol) -> Int64? {
let arr = Array(string.utf16)
var status = U_ZERO_ERROR
let parsed = unum_parseInt64(uformatter, arr, Int32(arr.count), nil, &status)
guard status.isSuccess else { return nil }
return parsed
withLockIfNeeded {
let arr = Array(string.utf16)
var status = U_ZERO_ERROR
let parsed = unum_parseInt64(uformatter, arr, Int32(arr.count), nil, &status)
guard status.isSuccess else { return nil }
return parsed
}
}

// `upperBound`: the utf-16 position in the string where the parse ends
Expand All @@ -44,25 +56,27 @@ internal final class ICULegacyNumberFormatter : @unchecked Sendable {
let arr = Array(string.utf16)
var status = U_ZERO_ERROR
var pos = Int32(0) // 0 == start, per ICU docs
let parsed = unum_parseInt64(uformatter, arr, Int32(arr.count), &pos, &status)
let parsed = withLockIfNeeded { unum_parseInt64(uformatter, arr, Int32(arr.count), &pos, &status) }
guard status.isSuccess else { return nil }
upperBound = Int(pos)
return parsed
}

func parseAsDouble(_ string: some StringProtocol) -> Double? {
let arr = Array(string.utf16)
var status = U_ZERO_ERROR
let parsed = unum_parseDouble(uformatter, arr, Int32(arr.count), nil, &status)
guard status.isSuccess else { return nil }
return parsed
withLockIfNeeded {
let arr = Array(string.utf16)
var status = U_ZERO_ERROR
let parsed = unum_parseDouble(uformatter, arr, Int32(arr.count), nil, &status)
guard status.isSuccess else { return nil }
return parsed
}
}

func parseAsDouble(_ string: some StringProtocol, upperBound: inout Int) -> Double? {
let arr = Array(string.utf16)
var status = U_ZERO_ERROR
var pos = Int32(0) // 0 == start, per ICU docs
let parsed = unum_parseDouble(uformatter, arr, Int32(arr.count), &pos, &status)
let parsed = withLockIfNeeded { unum_parseDouble(uformatter, arr, Int32(arr.count), &pos, &status) }
guard status.isSuccess else { return nil }
upperBound = Int(pos)
return parsed
Expand All @@ -74,47 +88,55 @@ internal final class ICULegacyNumberFormatter : @unchecked Sendable {
}

func parseAsDecimal(_ string: some StringProtocol, upperBound: inout Int) -> Decimal? {
var status = U_ZERO_ERROR
let arr = Array(string.utf16)
withLockIfNeeded {
let arr = Array(string.utf16)
var status = U_ZERO_ERROR

let formattable = ufmt_open(&status)
guard status.isSuccess else { return nil }
defer { ufmt_close(formattable) }
let formattable = ufmt_open(&status)
guard status.isSuccess else { return nil }
defer { ufmt_close(formattable) }

var pos = Int32(0) // 0 == start, per ICU docs
unum_parseToUFormattable(uformatter, formattable, arr, Int32(arr.count), &pos, &status)
guard status.isSuccess else { return nil }
upperBound = Int(pos)
var pos = Int32(0) // 0 == start, per ICU docs
unum_parseToUFormattable(uformatter, formattable, arr, Int32(arr.count), &pos, &status)
guard status.isSuccess else { return nil }
upperBound = Int(pos)

var len: Int32 = 0
guard let decNumChars = ufmt_getDecNumChars(formattable, &len, &status) else {
return nil
}
guard status.isSuccess else { return nil }
var len: Int32 = 0
guard let decNumChars = ufmt_getDecNumChars(formattable, &len, &status) else {
return nil
}
guard status.isSuccess else { return nil }

guard let str = String(validatingUTF8: decNumChars) else {
return nil
}
guard let str = String(validatingUTF8: decNumChars) else {
return nil
}

return Decimal(string: str)
return Decimal(string: str)
}
}

func format(_ v: Double) -> String? {
_withResizingUCharBuffer { buffer, size, status in
unum_formatDouble(self.uformatter, v, buffer, size, nil, &status)
withLockIfNeeded {
_withResizingUCharBuffer { buffer, size, status in
unum_formatDouble(self.uformatter, v, buffer, size, nil, &status)
}
}
}

func format(_ v: Int64) -> String? {
_withResizingUCharBuffer { buffer, size, status in
unum_formatInt64(self.uformatter, v, buffer, size, nil, &status)
withLockIfNeeded {
_withResizingUCharBuffer { buffer, size, status in
unum_formatInt64(self.uformatter, v, buffer, size, nil, &status)
}
}
}

func format(_ v: Decimal) -> String? {
_withResizingUCharBuffer { buffer, size, status in
let valueString = v.description
return unum_formatDecimal(uformatter, valueString, Int32(valueString.count), buffer, size, nil, &status)
withLockIfNeeded {
_withResizingUCharBuffer { buffer, size, status in
let valueString = v.description
return unum_formatDecimal(uformatter, valueString, Int32(valueString.count), buffer, size, nil, &status)
}
}
}

Expand All @@ -125,7 +147,7 @@ internal final class ICULegacyNumberFormatter : @unchecked Sendable {
case percent(NumberFormatStyleConfiguration.Collection)
case currency(CurrencyFormatStyleConfiguration.Collection, currencyCode: String)
case descriptive(DescriptiveNumberFormatConfiguration.Collection)

private typealias NumberCodingKeys = DefaultAssociatedValueCodingKeys1
private typealias PercentCodingKeys = DefaultAssociatedValueCodingKeys1
private typealias DescriptiveCodingKeys = DefaultAssociatedValueCodingKeys1
Expand Down Expand Up @@ -219,18 +241,16 @@ internal final class ICULegacyNumberFormatter : @unchecked Sendable {
}
}
}
return ICULegacyNumberFormatter(openedFormatter: formatter)

return ICULegacyNumberFormatter(openedFormatter: formatter, needsLock: icuType.isRuleBased)
}
}

private static let cache = FormatterCache<Signature, ICULegacyNumberFormatter>()
// lenient is only used for parsing
static func formatter(for type: NumberFormatType, locale: Locale, lenient: Bool = false) -> ICULegacyNumberFormatter? {
let sig = Signature(type: type, localeIdentifier: locale.identifier, lenient: lenient)
let formatter = try? ICULegacyNumberFormatter.cache.formatter(for: sig, creator: sig.createNumberFormatter)

return formatter
return try? ICULegacyNumberFormatter.cache.formatter(for: sig, creator: sig.createNumberFormatter)
}
}

Expand Down
8 changes: 8 additions & 0 deletions Sources/FoundationInternationalization/ICU/ICU+Enums.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,14 @@ extension UNumberFormatStyle {
static let spellout = UNUM_SPELLOUT
static let ordinal = UNUM_ORDINAL
static let scientific = UNUM_SCIENTIFIC
var isRuleBased: Bool {
switch self {
case .spellout, .ordinal:
true
default:
false
}
}
}

extension UNumberFormatAttribute {
Expand Down
12 changes: 12 additions & 0 deletions Tests/FoundationInternationalizationTests/Formatting/ByteCountFormatStyleTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,18 @@ private struct ByteCountFormatStyleTests {
#expect(Int64(4_200_000_000).formatted(.byteCount(style: .file, includesActualByteCount: true).locale(.init(identifier: "en_US")).attributed) == expected.attributedString)
}

@Test func concurrentZeroFormatting() async {
await withTaskGroup(of: Void.self) { group in
for _ in 0..<20 {
group.addTask {
for _ in 0..<50 {
_ = 0.formatted(.byteCount(style: .binary).locale(Locale(identifier: "en_US")))
}
}
}
}
}

#if !_pointerBitWidth(_32)
@Test func testEveryAllowedUnit() {
// 84270854: The largest unit supported currently is pb
Expand Down
Loading