fix: data race on Arrow v12 shared timestamp types (prewarm GetToTimeFunc) - #465
Open
TangoEnSkai wants to merge 1 commit into
Open
fix: data race on Arrow v12 shared timestamp types (prewarm GetToTimeFunc)#465TangoEnSkai wants to merge 1 commit into
TangoEnSkai wants to merge 1 commit into
Conversation
…Func) Arrow v12's TimestampType.GetToTimeFunc lazily caches the type's *time.Location via GetZone without synchronization (apache/arrow#38795, fixed only in later Arrow versions the driver cannot move to yet, see issue databricks#228). NewArrowRowScanner calls GetToTimeFunc on the shared arrow.FixedWidthTypes.Timestamp_us singleton for every result set, so concurrent queries race on that first call, as reported in issue databricks#179. Warm the cache for all four shared fixed-width timestamp singletons in a package init, before any concurrency is possible, so every later call is a plain read. This is the same workaround users currently have to apply in their own code; doing it in the driver removes the need to import Arrow (a transitive dependency) just to use databricks-sql-go safely from multiple goroutines. The new regression test fails under the race detector if the init is removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: TangoEnSkai <21152231+TangoEnSkai@users.noreply.github.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.
Context
close #179
Arrow v12's
TimestampType.GetToTimeFunclazily caches the type's*time.Location(viaGetZone, which writest.locwith no synchronization — apache/arrow#38795, fixed only in Arrow versions this driver can't move to yet, see #228).NewArrowRowScannercallsGetToTimeFuncon the sharedarrow.FixedWidthTypes.Timestamp_ussingleton for every result set, so the first concurrent queries race on that call. In the issue thread, @7phs posted theinit()prewarm workaround and @bombsimon asked for it to live in the driver instead of every consumer — this PR does exactly that.What
init()ininternal/rows/arrowbasedthat callsGetToTimeFunc()once on all four shared fixed-width timestamp singletons (Timestamp_s/_ms/_us/_ns), so the location cache is written before any concurrency is possible and every later call is a plain read.TestSharedTimestampGetToTimeFuncConcurrency, which hits the shared singletons from 32 goroutines. Verified both ways locally: with theinit()it passes under-race; with theinit()renamed away it fails withrace detected during execution of test.## Unreleased.Why
v16#228). Once warmed,GetZoneonly ever readst.loc, so the race is gone rather than hidden.init()in their own code just to usedatabricks-sql-gosafely from multiple goroutines.Timestamp_us, the oneNewArrowRowScanneruses) to match the workaround users already deploy and to keep any other shared-singleton use safe.Completion Criteria
go build ./...,go vet ./..., gofmt clean on touched filesgo test ./...)-racewith the fix, fails under-racewithout itNote for reviewers:
go test -race ./internal/rows/arrowbased/also reports pre-existing, unrelated races inTestCloudFetchIterator_CloseReleasesInFlightDownloads/TestCloudFetchIterator_CloseReleasesAfterRetry(acontainer/listrace in the CloudFetch iterator). Those fail identically onmainwithout this change.