remove try/catch from ~PooledConnection#89090
Conversation
clickhouse-gh
bot
added
the
pr-not-for-changelog
There was a problem hiding this comment.
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
tryLogCurrentExceptioncall that was logging caught exceptions
| ~PooledConnection() override | ||
| { | ||
| try | ||
| if (bool(response_stream)) | ||
| { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
robot-clickhouse-ci-1
added
the
pr-synced-to-cloud
remove try/catch from `~PooledConnection`
remove try/catch from `~PooledConnection`
Let's backport this to every release where #88668 exists |
robot-ch-test-poll2
added
the
pr-must-backport-synced
Cherry pick #89090 to 25.8: remove try/catch from `~PooledConnection`
Cherry pick #89090 to 25.10: remove try/catch from `~PooledConnection`
robot-clickhouse
added
the
pr-backports-created
Backport #89090 to 25.10: remove try/catch from `~PooledConnection`
Backport #89090 to 25.8: remove try/catch from `~PooledConnection`
Changelog category (leave one):