Repository navigation
fix(common): make the query timer's start time atomic - #1556
Merged
Merged
Conversation
With a query timeout, the thread waiting on a task reads the statement's timer in getTimeoutRemainingInMS, while the thread running IMPORT DATABASE restarts it for each imported statement (resetActiveQuery and startTimer in executeNoLock). Timer's start time was a plain time_point, so this was a data race. TSan reports it on main in TaskSchedulerTest.TimeoutInterruptsStatementStartedAfterDeadline and in an import of many short statements. The start time is now a std::atomic, stored with release and loaded with acquire. It has the same size and alignment as the plain time_point (a static_assert pins this), so ClientContext's layout does not change. ActiveQuery::reset() no longer zeroes the timer: a waiter reading between the reset and startTimer() could see a long-expired deadline and interrupt the statement that was starting. TimeMetric's constructor drops a redundant timer = Timer(), which the atomic no longer allows. INTERRUPT_RECHECK_INTERVAL_MS gets a comment on what it bounds and why it is not shorter. Co-Authored-By: Claude Opus 5.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.
This work was produced with the help of language models.
Follow-up to #1482 for the two points in the review comment.
Timer race. With a query timeout, the waiting thread reads the statement's timer while the thread running
IMPORT DATABASErestarts it for each imported statement, and the start time was a plaintime_point. Onmain, TSan reports the race inTaskSchedulerTest.TimeoutInterruptsStatementStartedAfterDeadline(3 of 3 runs) and in an import of 1500RETURN 1;statements on one thread with a 20 ms timeout.ActiveQuery::reset()also zeroed the timer beforestartTimer()set it again, so a waiter reading in between would see a long-expired deadline and interrupt the statement that was starting.Fix. The start time is now a
std::atomic, stored with release and loaded with acquire, which compile to the same plain moves as before on x86-64. Astatic_assertpins its size and alignment to the plaintime_point, soClientContext's layout does not change.reset()no longer zeroes the timer: a waiter reading beforestartTimer()now sees the previous statement's start, which has expired only if more than the timeout has passed since it. The atomic makesTimernon-copyable, so a redundanttimer = Timer()inTimeMetric's constructor is gone; nothing in the tree, the extensions or the Python driver copies aTimer.Re-check interval. Completion wakes the waiter at once, so the interval delays no wakeup. Past the deadline the waiter reads the clock every 100 ms, sees a statement that started after the deadline within 100 ms, and then sleeps until that statement's own deadline, so only timeouts under 100 ms can overrun. A scratch test that started the next statement at 13 points of the cycle measured this in a Release build: 150 and 300 ms timeouts were interrupted within 1 ms of their deadline, 20 and 50 ms ones at most 96 ms after the statement started. 1000 ms would let sub-second timeouts overrun by up to a second; 10 ms would help only timeouts under 100 ms, with ten times the wakeups. The comment on
INTERRUPT_RECHECK_INTERVAL_MSnow says this.Tests. No new test: the scheduler test above reports the race under TSan on every run, so the nightly TSan job will flag it if it comes back, and a timing assertion would be flaky on loaded CI runners. With the change, TSan reports nothing in those two cases or in
TaskSchedulerTest.*(3 runs each). Release ctest: 3439 passed, 0 failed; the import, export, large-copy, GDS and timeout tests also pass withMAX_NUM_THREADS=1.