Guard tune_timeout_for_session_needs_pause from stale pause_until - #5784
Guard tune_timeout_for_session_needs_pause from stale pause_until#5784rahim-kanji wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (4)
🧰 Additional context used📓 Path-based instructions (1)Class names must use `PascalCase` with protocol prefixes such as `MySQL_`, `PgSQL_`, and `ProxySQL_`.📄 CodeRabbit inference engine (CLAUDE.md) Files:
🪛 Cppcheck (2.21.0)lib/Base_Thread.cpp[warning] 46-46: If memory allocation fails, then there is a possible null pointer dereference (nullPointerOutOfMemory) 🔇 Additional comments (3)
📝 WalkthroughWalkthroughThree files refine pause scheduling. ChangesPause handling across polling and session connections
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change prevents expired connection-retry pauses from delaying worker polling and clears stale pause state after successful backend connections for MySQL and PostgreSQL. No current merge-blocking risk remains. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces resets for pause_until during server connection status changes in MySQL and PostgreSQL sessions and refactors the timeout tuning logic in Base_Thread.cpp. Review feedback highlights that the commented-out logic for expired pauses should be enabled to prevent thread blocking. Additionally, the reviewer pointed out a unit mismatch where microsecond values were being assigned to a millisecond-based timeout and provided a code suggestion to correct the calculation and ensure proper polling behavior.
| if (myds->sess->pause_until > curtime) { | ||
| // Future pause: align poll_timeout to the pause expiration. | ||
| if (thr->mypolls.poll_timeout == 0 || (myds->sess->pause_until - curtime < thr->mypolls.poll_timeout)) { | ||
| thr->mypolls.poll_timeout = myds->sess->pause_until - curtime; | ||
| proxy_debug(PROXY_DEBUG_MYSQL_CONNECTION, 7, "Session=%p , poll_timeout=%u , pause_until=%llu , curtime=%llu\n", myds->sess, thr->mypolls.poll_timeout, | ||
| myds->sess->pause_until, curtime); | ||
| } | ||
| } | ||
| /* Do we need immediately wake up poll() because of an already expired pause? | ||
| else { | ||
| // pause_until > 0 (caller checked) but <= curtime: pause has already expired. | ||
| // Wake poll() immediately rather than computing (pause_until - curtime) | ||
| if (thr->mypolls.poll_timeout == 0 || thr->mypolls.poll_timeout > 1) { | ||
| thr->mypolls.poll_timeout = 1; | ||
| } | ||
| }*/ | ||
| } |
There was a problem hiding this comment.
The logic for handling expired pauses should be enabled, and a unit mismatch in the timeout calculation needs to be addressed:
- Expired Pauses: The commented-out
elseblock should be active. Ifpause_until <= curtime, the session is ready for processing. By not updatingpoll_timeoutto a small value (like 1ms), the thread may block for the default timeout (2000ms), causing the freeze described in the PR summary. - Unit Mismatch:
pause_until - curtimeis in microseconds, butpoll_timeoutis compared downstream against a millisecond value (2000 ms). Ifpoll_timeoutis set to microseconds, the comparisontimeout < 2000will fail for any pause longer than 2ms, causing the tuning to be ignored. The difference should be converted to milliseconds.
if (myds->sess->pause_until > curtime) {
// Future pause: align poll_timeout to the pause expiration.
// Convert microseconds to milliseconds (ceiling division)
unsigned int timeout_ms = (myds->sess->pause_until - curtime + 999) / 1000;
if (thr->mypolls.poll_timeout == 0 || timeout_ms < (unsigned int)thr->mypolls.poll_timeout) {
thr->mypolls.poll_timeout = timeout_ms;
proxy_debug(PROXY_DEBUG_MYSQL_CONNECTION, 7, "Session=%p , poll_timeout=%u , pause_until=%llu , curtime=%llu\n", myds->sess, thr->mypolls.poll_timeout,
myds->sess->pause_until, curtime);
}
} else {
// pause_until > 0 (caller checked) but <= curtime: pause has already expired.
// Wake poll() immediately rather than computing (pause_until - curtime)
if (thr->mypolls.poll_timeout == 0 || thr->mypolls.poll_timeout > 1) {
thr->mypolls.poll_timeout = 1;
}
}
tune_timeout_for_session_needs_pause computed (pause_until - curtime) as unsigned subtraction without checking if pause_until was still in the future. When pause_until <= curtime (stale), the result underflowed to ~1.8e19, which was then assigned to a signed int poll_timeout, becoming a large negative integer. The downstream ttw calculation at PgSQL_Thread.cpp:3210 compares this against (unsigned int)pgsql_thread___poll_timeout. Signed-to-unsigned promotion turned the negative value into a huge unsigned, the comparison took the wrong branch, and ttw fell back to the default poll_timeout (2000 ms). The worker thread then blocked in poll() for up to 2 seconds, freezing all sessions on that thread.
81f4798 to
1430f50
Compare
|
Stale-pause analysis — the condition is reachable, but low severityReviewing whether Entry guard ( if (unlikely(myds->sess->pause_until > 0)) {
tune_timeout_for_session_needs_pause<T>(myds);
}So the question reduces to: can a session reach Yes — there is a real lifecycle gapIn // 2994: connect attempt fails → set a FUTURE pause and return
if (mybe->server_myds->myconn==NULL) {
pause_until = thread->curtime + mysql_thread___connect_retries_delay*1000; // future
*_rc=1;
return false;
}
...
// 3008: NEXT iteration — connection is now ASYNC_IDLE (succeeded)
if (mybe->server_myds->myconn->async_state_machine==ASYNC_IDLE) {
st = previous_status.top(); previous_status.pop();
NEXT_IMMEDIATE_NEW(st); // == set_status(st); return true; ← pause_until NOT cleared
}
The usual "AfterPoll clears it via the handler" reasoning does not cover this case, because the handler's own terminal transition is precisely what leaves But the impact is smaller than the PR description claimsThe PR's "huge unsigned → poll() frozen" chain is not quite what happens, because
Real-world effect: a backend connect-retry that succeeds at the retry-delay boundary leaves a stale Verdict
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.0 #5784 +/- ##
==========================================
- Coverage 31.51% 27.05% -4.46%
==========================================
Files 435 159 -276
Lines 142709 82755 -59954
Branches 12374 22530 +10156
==========================================
- Hits 44968 22387 -22581
+ Misses 89287 54067 -35220
+ Partials 8454 6301 -2153 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Summary
tune_timeout_for_session_needs_pause computed (pause_until - curtime) as unsigned subtraction without checking if pause_until was still in the future. When pause_until <= curtime (stale), the result underflowed, which was then assigned to a signed int poll_timeout, becoming a large negative integer.
The downstream ttw calculation compares this against (unsigned int)pgsql_thread___poll_timeout. Signed-to-unsigned promotion turned the negative value into a huge unsigned, the comparison took the wrong branch, and ttw fell back to the default poll_timeout (2000 ms). The worker thread then blocked in poll() for up to 2 seconds, freezing all sessions on that thread.
Summary by CodeRabbit