New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cancel PipelineExecutor properly in case of exception in spawnThreads #60499
Conversation
This is an automated comment for commit 1b8ae25 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
|
{ | ||
/// spawnThreads can throw an exception, for example CANNOT_SCHEDULE_TASK. | ||
/// We should cancel execution properly before rethrow. | ||
cancel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we rather wrap the entire executeImpl()
body in try/catch and call cancel()
accordingly? I find this fix too narrow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But spawnThreads
is the only place where we don't cancel execution in case of an exception. Other code either doesn't throw or handles exceptions correctly. Ideally, spawnThreads
should not throw, but by now it's not possible: https://github.com/ClickHouse/clickhouse-core-incidents/issues/10#issuecomment-1816103879
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No pressure regarding merging the fix - feel free to, but ...
But spawnThreads is the only place ...
It seems, it's a strong statement which can change overtime, and I don't see an easy way to conclude it from the code. But the function should hold the contract, i.e. - in case of exception, cancel whatever background jobs/processes it's doing. If it'd be implemented in the proposed way, then we'd not have the escalation in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I just realized that there is a code above:
SCOPE_EXIT_SAFE(
if (!finished_flag)
{
finish();
if (pool)
pool->wait();
}
);
And AFAIU finished_flag
can be false
only in case of an exception in executeImpl()
, but we should call cancel()
instead of finish()
in this case, it's similar to #52533. So, let's do it
finish(); | ||
/// If finished_flag is not set, there was an exception. | ||
/// Cancel execution in this case. | ||
cancel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unclear if there was any reason to call only finish()
here. The more I read code around, the more questions I have regarding the contracts. In general, looks ok but can't say anything in particular from my current understanding 😕
…5ec2c4138cd1d3167a3ad3a8d2e43 Cherry pick #60499 to 23.8: Cancel PipelineExecutor properly in case of exception in spawnThreads
…exception in spawnThreads
…25ec2c4138cd1d3167a3ad3a8d2e43 Cherry pick #60499 to 23.12: Cancel PipelineExecutor properly in case of exception in spawnThreads
… exception in spawnThreads
…5ec2c4138cd1d3167a3ad3a8d2e43 Cherry pick #60499 to 24.1: Cancel PipelineExecutor properly in case of exception in spawnThreads
…exception in spawnThreads
…5ec2c4138cd1d3167a3ad3a8d2e43 Cherry pick #60499 to 24.2: Cancel PipelineExecutor properly in case of exception in spawnThreads
…exception in spawnThreads
Backport #60499 to 24.1: Cancel PipelineExecutor properly in case of exception in spawnThreads
Backport #60499 to 24.2: Cancel PipelineExecutor properly in case of exception in spawnThreads
Backport #60499 to 23.12: Cancel PipelineExecutor properly in case of exception in spawnThreads
Backport #60499 to 23.8: Cancel PipelineExecutor properly in case of exception in spawnThreads
…5ec2c4138cd1d3167a3ad3a8d2e43 Cherry pick #60499 to 23.3: Cancel PipelineExecutor properly in case of exception in spawnThreads
…exception in spawnThreads
Backport #60499 to 23.3: Cancel PipelineExecutor properly in case of exception in spawnThreads
Changelog category (leave one):
Very similar to #57104, it could lead to crash in fiber because RemoteQueryExecutor is not cancelled properly.