Skip to content

Initialize ParallelReadBuffer after construction#37726

Merged
antonio2368 merged 3 commits intomasterfrom
fix-parallel-download-exception
Jun 2, 2022
Merged

Initialize ParallelReadBuffer after construction#37726
antonio2368 merged 3 commits intomasterfrom
fix-parallel-download-exception

Conversation

@antonio2368
Copy link
Copy Markdown
Member

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Some of the readers can throw exceptions so initializing the ParallelReadBuffer in constructor can lead to undefined state if the exception happens while the readers are being added.

cc: @davenger @Avogar

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

@robot-ch-test-poll robot-ch-test-poll added the pr-not-for-changelog This PR should not be mentioned in the changelog label Jun 1, 2022
@rschu1ze rschu1ze self-assigned this Jun 1, 2022
Comment on lines +52 to +60
try
{
addReaders();
}
catch (const Exception &)
{
finishAndWait();
throw;
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this solution seems a bit nicer @rschu1ze 😄

@antonio2368 antonio2368 merged commit 46abbac into master Jun 2, 2022
@antonio2368 antonio2368 deleted the fix-parallel-download-exception branch June 2, 2022 10:35
@yokofly yokofly mentioned this pull request Dec 15, 2025
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants