Fix geocoding metadata resolution under SwiftPM and per-language statement poisoning - #453
Merged
Merged
Conversation
The shared NBPhoneNumberUtil is reachable from any thread, and three pieces of state it owns or reaches were not protected: - NBMetadataHelper built its region-to-calling-code table lazily with no synchronization, so concurrent first use raced on the ivar write. - NBPhoneNumberOfflineGeocoder looked up its per-language helper in an NSCache and inserted on a miss, so concurrent callers could each open their own SQLite connection for the same language. - NBRegularExpressionCache held @synchronized(self) across pattern compilation, serializing every caller behind an unrelated pattern being built, including callers that would have hit the cache. Lock the metadata table with os_unfair_lock, move the geocoder's lookup and insertion under one lock, and narrow the regex cache's lock to the lookup and the insertion so compilation happens outside it. Two threads racing on the same new pattern may now each compile it; the duplicate is discarded on insertion. This is a prerequisite for declaring the Swift facades Sendable: that claim rests on this locking rather than on anything the compiler checks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…anguage Two failures that both surface as a correct-looking country name instead of a locality, with no error and no crash. The metadata bundle was never found under Swift Package Manager. NBGeocoderMetaDataHelper appended GeocodingMetaData.bundle to -[NSBundle bundleForClass:].resourceURL, which is where CocoaPods, Carthage, and manual integration put the databases. SwiftPM nests them inside a generated wrapper bundle, and emits that wrapper flat up to Xcode 26 and macOS-structured from Xcode 27. No database was opened, so every lookup fell through to countryNameForNumber:. Apply the same search the carrier and timezone mappers already use, covering both layouts. 191c758 fixed this class of bug for carrier and timezone metadata and recorded that geocoding resolves its bundle differently and was unaffected. It resolves it differently, but it was affected: only the Objective-C tests covered geocoding, and they inject a test bundle, so the shipped path was never exercised. Separately, a lookup for a country a language's database does not carry disabled that language permanently. Only en.db has worldwide coverage, so preparing the query against a missing table fails; the failed statement was left in place and every later lookup short-circuited on a NULL statement. A Korean-locale app that geocoded one US number lost city-level Korean geocoding for the rest of the process. Treat a failed prepare as "no data for this country", and re-prepare on the next call. The two Swift facade tests that asserted "United States" for a Mountain View number were pinning the first bug; they now assert the locality. The new suite covers localization, the user-region split, and a regression test for the poisoned statement that fails before this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Summary
Split out of #452 so the fixes are not blocked behind that PR's breaking Swift 6 changes. #452 keeps the same two commits and should be rebased once this merges.
Upstream / Metadata
The bugs
Metadata was never found under SwiftPM.
NBGeocoderMetaDataHelperappendedGeocodingMetaData.bundleto-[NSBundle bundleForClass:].resourceURL, which is where CocoaPods, Carthage, and manual integration put the databases. SwiftPM nests them inside a generated wrapper bundle. No database was opened, so every lookup fell through tocountryNameForNumber:with no error and no crash:The database has carried
1650253 → Mountain View, CAall along.191c758 fixed this class of bug for carrier and timezone metadata and recorded that geocoding resolves its bundle differently and was unaffected. It does resolve it differently, but it was affected. Only the Objective-C tests covered geocoding and they inject a test bundle, so the shipped path was never exercised — and the two Swift facade tests that did run had pinned
"United States"as the expected value. Those assertions are corrected here.A lookup for an uncovered country disabled that language permanently. Only
en.dbhas worldwide coverage; the others carry their own country. Preparing the query against a missing table fails, and the failed statement was left in place, so every later lookup in that language short-circuited on aNULLstatement. A Korean-locale app that geocoded one US number lost city-level Korean geocoding for the rest of the process. The regression test for this fails before the fix and passes after it, verified by reverting the fix in place.The first commit is a prerequisite: the geocoder's per-language helper cache was a check-then-insert, so concurrent callers could each open their own SQLite connection for the same language. It also locks
NBMetadataHelper's lazily built region table, which had no synchronization at all, and narrows the regular-expression cache's lock so compilation happens outside it.Compatibility
No API changes. Two behavioral changes worth calling out in release notes:
libPhoneNumberGeocodingstart receiving locality descriptions where they previously received country names. Apps that display the result directly will show different text.Parity Checks
swift scripts/checkUpstreamTestParity.swift— no gapsswift scripts/checkVersionConsistency.swiftTests
swift test— 231 XCTest + 7 Swift Testing, 0 failuresLC_ALL=ko_KR.UTF-8 LANG=ko_KR.UTF-8 swift testswift build -c releasexcodebuild test -scheme libPhoneNumber— TEST SUCCEEDEDxcodebuild test -scheme libPhoneNumberGeocoding— TEST SUCCEEDEDgit diff --checkNotes
PhoneNumberGeocodingFacadeTestssuite covers localization, the user-region split, the uncovered-country regression, and concurrent lookups through the shared geocoder.NBGeocoderMetaDataHelper.defaultMetadataBundleis exposed so integrators can check whether the metadata resolved at all, rather than silently receiving country-level results.CHANGELOG.mdentry: that file is introduced by Fix geocoding metadata resolution and adopt Swift 6 concurrency #452. If this ships first as a patch, the entries for these fixes should move under a released heading when Fix geocoding metadata resolution and adopt Swift 6 concurrency #452 is rebased.🤖 Generated with Claude Code