24.8.14 Backport of #75474 - Stricter checks when attaching/detaching threads to groups#2016
Open
ilejn wants to merge 1 commit into
Open
24.8.14 Backport of #75474 - Stricter checks when attaching/detaching threads to groups#2016ilejn wants to merge 1 commit into
ilejn wants to merge 1 commit into
Conversation
Stricter checks when attaching/detaching threads to groups Signed-off-by: Ilya Golshtein <igolshtein@altinity.com>
Collaborator
|
This is an automated comment for commit 5cd9a99 with description of existing statuses. It's updated for the latest CI running ✅ Click here to open a full report in a separate page Successful checks
|
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
2 participants
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.
Changelog category (leave one):
Closes ClickHouse#59114
In https://s3.amazonaws.com/clickhouse-test-reports/74014/dcdccd54f13e74c5018f6d40e5a4caa77858dc22/stress_test__debug_.html
CurrentThread::getGroup()returned nullptr when called fromTCPHandler::processOrdinaryQuery. This means that calls toCurrentThread::attachToGroup/detachFromGroupIfNotDetachedare not properly nested: someone detached a thread that they didn't attach. (The thread is expected to be attached to a group because the caller ofprocessOrdinaryQueryholds a QueryScope for the duration of the call.)Almost all calls to
CurrentThread::attachToGroupIfDetached/detachFromGroupIfNotDetachedfollow this pattern that looks incorrect:If the thread is already attached to a group, it'll end up detached after this function returns. Normally such
thread_funcis immediately scheduled on a thread pool; then the thread shouldn't be already attached to a group (... or so I'm assuming; there were no asserts about this, so CI on this PR may prove this analysis wrong). But some code sites sometimes call such function directly, e.g.Aggregator::prepareBlocksAndFillTwoLevelImpl. AFAICT, this would incorrectly detach the thread from the thread group. I'm guessing this is what caused these test failures (but I didn't try to reproduce this and don't have an explanation for why the crash happens so rarely.)This PR replaces this pattern with a more correct one, with a bunch of asserts, wrapped in a RAII thing.
Most changes are mechanical. Most substantial ones are in
ThreadStatus.h,ThreadStatusExt.cpp, andProfileEventsExt.cpp.CI/CD Options
Exclude tests:
Regression jobs to run: