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 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (2)**/*.{cpp,h,hpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
{lib,src}/**/*.{cpp,h,hpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🔇 Additional comments (3)
📝 WalkthroughWalkthroughThree files update pause-until delay handling: ChangesPause delay handling across thread polling and session connections
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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.
rahim-kanji
force-pushed
the
v3.0_fix-stale-pause-until
branch
from
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
|
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