Skip to content

Conversation

@mstetsyuk
Copy link
Member

@mstetsyuk mstetsyuk commented Oct 27, 2025

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

@clickhouse-gh
Copy link

clickhouse-gh bot commented Oct 27, 2025

Workflow [PR], commit [51eae77]

Summary:

@clickhouse-gh clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Oct 27, 2025
@mstetsyuk mstetsyuk requested a review from Copilot October 27, 2025 20:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes the try-catch block from the ~PooledConnection() destructor, eliminating the exception handling that was previously suppressing and logging any exceptions thrown during connection cleanup.

Key Changes:

  • Removed try-catch wrapper from the destructor body
  • Eliminated the tryLogCurrentException call that was logging caught exceptions

Comment on lines 439 to 442
~PooledConnection() override
{
try
if (bool(response_stream))
{
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

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

Removing the try-catch block from a destructor is risky because destructors are implicitly noexcept in C++11 and later. If any exception is thrown during the cleanup operations (e.g., from response_stream operations or metrics updates), the program will call std::terminate() and crash. Consider either keeping the try-catch to handle exceptions gracefully, or ensure all operations within the destructor are guaranteed not to throw.

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

Adding try catch in every d-tors makes a noise.
You mute exception in d-tor not by choice but out of necessity, as a result you mute an undefined wide set of exceptions. Much better approach is to guarantee that all actions in d-tor are not able to throw.

@CheSema CheSema enabled auto-merge October 28, 2025 10:02
@CheSema CheSema added this pull request to the merge queue Oct 28, 2025
Merged via the queue into master with commit da07499 Oct 28, 2025
124 checks passed
@CheSema CheSema deleted the mstetsyuk-patch-1 branch October 28, 2025 12:18
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Oct 28, 2025
zvonand pushed a commit to Altinity/ClickHouse that referenced this pull request Oct 30, 2025
zvonand pushed a commit to Altinity/ClickHouse that referenced this pull request Oct 31, 2025
@rienath
Copy link
Member

rienath commented Nov 27, 2025

raufsdunamalijevs@Raufs-Macbook trash % dataplanectl cidb find pr 88668

    PR  CURRENT REPO PR  UPSTREAM REPO PR  VERSION       COMMIT                                    GIT REF      
 88668                   ✅                25.10.1.5838  56f27aa1de46842d12ced000a086badcb438f1dd  master       
 88668                   ✅                25.8.1.8538   749cae4f85a99b5aa280cbf4c65bf9ffd18ffe5c  release/25.8 
raufsdunamalijevs@Raufs-Macbook trash % dataplanectl cidb find pr 89090 


    PR  CURRENT REPO PR  UPSTREAM REPO PR  VERSION      COMMIT                                    GIT REF 
 89090                   ✅                25.11.1.704  33de285cfe44fad60314af3541d9c282a458aad4  master  

Let's backport this to every release where #88668 exists

@robot-clickhouse robot-clickhouse added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Nov 27, 2025
clickhouse-gh bot added a commit that referenced this pull request Nov 27, 2025
Backport #89090 to 25.10: remove try/catch from `~PooledConnection`
rienath added a commit that referenced this pull request Nov 27, 2025
Backport #89090 to 25.8: remove try/catch from `~PooledConnection`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR pr-not-for-changelog This PR should not be mentioned in the changelog pr-synced-to-cloud The PR is synced to the cloud repo v25.8-must-backport v25.10-must-backport

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants