make `HTTPChunkedStreamBuf::isComplete` noexcept by mstetsyuk · Pull Request #88668 · ClickHouse/ClickHouse

mstetsyuk · 2025-10-16T14:23:51Z

Changelog category (leave one):

Critical Bug Fix (crash, data loss, RBAC) or LOGICAL_ERROR

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

A Poco::TimeoutException exception thrown from Poco::Net::HTTPChunkedStreamBuf::readFromDevice leads to a crash with SIGABRT.

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

clickhouse-gh · 2025-10-16T14:24:20Z

Workflow [PR], commit [655054a]

Summary: ❌

job_name	test_name	status	info
Stateless tests (amd_binary, old analyzer, s3 storage, DatabaseReplicated, parallel)		failure
	01396_inactive_replica_cleanup_nodes_zookeeper	FAIL	cidb
Stateless tests (amd_tsan, s3 storage, parallel)		failure
	03237_insert_sparse_columns_mem	FAIL	cidb
Bugfix validation (functional tests)		failure
Stress test (arm_asan)		failure
	Server died	FAIL	cidb
	Hung check failed, possible deadlock found (see hung_check.log)	FAIL	cidb
	Killed by signal (in clickhouse-server.log)	FAIL	cidb
	Fatal message in clickhouse-server.log (see fatal_messages.txt)	FAIL	cidb
	Killed by signal (output files)	FAIL	cidb

CheSema · 2025-10-16T15:23:47Z

bool HTTPChunkedStreamBuf::isComplete(bool read_from_device_to_check_eof)

here we should catch more exceptions.

src/Common/HTTPConnectionPool.cpp

alexey-milovidov

.

alexey-milovidov · 2025-10-16T15:26:12Z

02537_distributed_loosing_files_after_exception

Should be renamed, loosing -> losing.

alexey-milovidov · 2025-10-16T15:26:36Z

Exception thrown

What kind of exception? Is it possible to make a test?

mstetsyuk · 2025-10-16T17:11:24Z

Is it possible to make a test?

Made HTTPChunkedStreamBuf::isComplete noexcept and added a unit test for it.

What kind of exception?

Poco::TimeoutException.

Here's the stack trace:

[ 125 ] () <Fatal> BaseDaemon: Poco::Exception. Code: 1000, e.code() = 0, Timeout, Stack trace (when copying this message, always include the lines below):

0. ./ci/tmp/build/./base/poco/Net/src/SocketImpl.cpp:327: Poco::Net::SocketImpl::receiveBytes(void*, int, int) @ 0x000000001f96800d
1. ./base/poco/Net/src/StreamSocket.cpp:135: Poco::Net::HTTPSession::receive(char*, int) @ 0x000000001f94d534
2. ./base/poco/Net/src/HTTPSession.cpp:226: Poco::Net::HTTPChunkedStreamBuf::readFromDevice(char*, long) @ 0x000000001f93ec49
3. ./base/poco/Net/src/HTTPChunkedStream.cpp:151: DB::EndpointConnectionPool<Poco::Net::HTTPClientSession>::PooledConnection::~PooledConnection() @ 0x00000000139a0807
4. ./contrib/llvm-project/libcxx/include/__memory/shared_ptr.h:156: (anonymous namespace)::ReadBufferFromSessionResponse::~ReadBufferFromSessionResponse() (.75713a3d1978382ea5f32a923074deb4) @ 0x0000000015904d77
5. ./contrib/llvm-project/libcxx/include/__memory/unique_ptr.h:80: DB::BuilderRWBufferFromHTTP::create(Poco::Net::HTTPBasicCredentials const&) @ 0x0000000015904633
6. ./ci/tmp/build/./src/Storages/StorageSharedMergeTree.cpp:735: DB::StorageSharedMergeTree::getCacheHint(String const&) const @ 0x0000000019234edd
7. ./ci/tmp/build/./src/Storages/StorageSharedMergeTree.cpp:5440: DB::StorageSharedMergeTree::tryUpdateDiskMetadataCacheForPart(String const&) const @ 0x000000001924334c
8. ./ci/tmp/build/./src/Storages/SharedMergeTree/PartsKillerThread.cpp:233: DB::KillPartsTask::tryKillPart(String const&, long, bool) @ 0x0000000019e0b142
9. ./ci/tmp/build/./src/Storages/SharedMergeTree/PartsKillerThread.cpp:455: void std::__function::__policy_invoker<void ()>::__call_impl[abi:ne190107]<std::__function::__default_alloc_func<DB::PartsKillerThread::run()::$_0, void ()>>(std::__function::__policy_storage const*) @ 0x0000000019e1ef4a
10. ./contrib/llvm-project/libcxx/include/__functional/function.h:716: ? @ 0x000000001369b22b
11. ./contrib/llvm-project/libcxx/include/__type_traits/invoke.h:117: void std::__function::__policy_invoker<void ()>::__call_impl[abi:ne190107]<std::__function::__default_alloc_func<ThreadFromGlobalPoolImpl<false, true>::ThreadFromGlobalPoolImpl<void (ThreadPoolImpl<ThreadFromGlobalPoolImpl<false, true>>::ThreadFromThreadPool::*)(), ThreadPoolImpl<ThreadFromGlobalPoolImpl<false, true>>::ThreadFromThreadPool*>(void (ThreadPoolImpl<ThreadFromGlobalPoolImpl<false, true>>::ThreadFromThreadPool::*&&)(), ThreadPoolImpl<ThreadFromGlobalPoolImpl<false, true>>::ThreadFromThreadPool*&&)::'lambda'(), void ()>>(std::__function::__policy_storage const*) @ 0x00000000136a25a6
12. ./contrib/llvm-project/libcxx/include/__functional/function.h:716: ? @ 0x0000000013698212
13. ./contrib/llvm-project/libcxx/include/__type_traits/invoke.h:117: void* std::__thread_proxy[abi:ne190107]<std::tuple<std::unique_ptr<std::__thread_struct, std::default_delete<std::__thread_struct>>, void (ThreadPoolImpl<std::thread>::ThreadFromThreadPool::*)(), ThreadPoolImpl<std::thread>::ThreadFromThreadPool*>>(void*) @ 0x000000001369fcda
14. ? @ 0x0000000000094ac3
15. ? @ 0x00000000001268c0
 (version 25.8.1.8490 (official build))

…stributed_losing_files_after_exception

alexey-milovidov

LGTM

alexey-milovidov · 2025-10-16T22:31:44Z

@mstetsyuk, just in case, we have something similar, reproduced in this PR:

[ 166150 ] {02cc0dfd-a5fc-4a24-b69d-dc1b0962ba1f} <Fatal> : Logical error: \'ReadBuffer is canceled. Can\'t read from it.\'.
 [ 166150 ] {02cc0dfd-a5fc-4a24-b69d-dc1b0962ba1f} <Fatal> : Stack trace (when copying this message, always include the lines below):
 [ 5596 ] {} <Fatal> BaseDaemon: ########## Short fault info ############
 [ 5596 ] {} <Fatal> BaseDaemon: (version 25.10.1.3175, build id: BA7ADB117D95ED0E9FE129DB9CE80E4556B7608F, git hash: 9183bab3094f8510003e12f1f18ef7d6502bbbef, architecture: aarch64) (from thread 166150) Received signal 6
 [ 5596 ] {} <Fatal> BaseDaemon: Signal description: Aborted
 [ 5596 ] {} <Fatal> BaseDaemon: 
 [ 5596 ] {} <Fatal> BaseDaemon: Stack trace: 0x0000ffef51f2f1f1 0x0000ffef51eea67c 0x0000ffef51ed7130 0x0000ab39e31bfd60 0x0000ab39e31c02f8 0x0000ab39e3396e98 0x0000ab39ed0f1fc0 0x0000ab3a083a0794 0x0000ab3a045a9590 0x0000ab3a0458e024 0x0000ab3a044ccdb4 0x0000ab3a044c91ec 0x0000ab3a044cf4c0 0x0000ab3a044e9530 0x0000ab3a044e3bf4 0x0000ab3a044dc14c 0x0000ab3a044ff248 0x0000ab3a0481a61c 0x0000ab3a04732974 0x0000ab3a04730fac 0x0000ab39eca8c994 0x0000ab39ed0d35c0 0x0000ab39ed74fab0 0x0000ab39ed6185c0 0x0000ab39ed74cea8 0x0000ab39ed638b2c 0x0000ab39ed5efa1c 0x0000ab39e65eab8c 0x0000ab39e65ea620 0x0000ab39e345cff8 0x0000ab39e3468b9c 0x0000ab39e34582b4 0x0000ab39e3464cb0 0x0000ab39d52dc834 0x0000ffef51f2d5b8 0x0000ffef51f95edc
 [ 5596 ] {} <Fatal> BaseDaemon: ########################################
 [ 5596 ] {} <Fatal> BaseDaemon: (version 25.10.1.3175, build id: BA7ADB117D95ED0E9FE129DB9CE80E4556B7608F, git hash: 9183bab3094f8510003e12f1f18ef7d6502bbbef) (from thread 166150) (query_id: 02cc0dfd-a5fc-4a24-b69d-dc1b0962ba1f) (query: BACKUP TABLE data TO S3(\'http://localhost:11111/test/backups/test_oqeb1o4a/use_same_s3_credentials_for_base_backup_base_base\', \'test\', \'[HIDDEN]\')) Received signal Aborted (6)
 [ 5596 ] {} <Fatal> BaseDaemon: 
 [ 5596 ] {} <Fatal> BaseDaemon: Stack trace: 0x0000ffef51f2f1f1 0x0000ffef51eea67c 0x0000ffef51ed7130 0x0000ab39e31bfd60 0x0000ab39e31c02f8 0x0000ab39e3396e98 0x0000ab39ed0f1fc0 0x0000ab3a083a0794 0x0000ab3a045a9590 0x0000ab3a0458e024 0x0000ab3a044ccdb4 0x0000ab3a044c91ec 0x0000ab3a044cf4c0 0x0000ab3a044e9530 0x0000ab3a044e3bf4 0x0000ab3a044dc14c 0x0000ab3a044ff248 0x0000ab3a0481a61c 0x0000ab3a04732974 0x0000ab3a04730fac 0x0000ab39eca8c994 0x0000ab39ed0d35c0 0x0000ab39ed74fab0 0x0000ab39ed6185c0 0x0000ab39ed74cea8 0x0000ab39ed638b2c 0x0000ab39ed5efa1c 0x0000ab39e65eab8c 0x0000ab39e65ea620 0x0000ab39e345cff8 0x0000ab39e3468b9c 0x0000ab39e34582b4 0x0000ab39e3464cb0 0x0000ab39d52dc834 0x0000ffef51f2d5b8 0x0000ffef51f95edc
 [ 5596 ] {} <Fatal> BaseDaemon: 3. ? @ 0x000000000007f1f1
 [ 5596 ] {} <Fatal> BaseDaemon: 4. ? @ 0x000000000003a67c
 [ 5596 ] {} <Fatal> BaseDaemon: 5. ? @ 0x0000000000027130
 [ 5596 ] {} <Fatal> BaseDaemon: 6. ./ci/tmp/build/./src/Common/Exception.cpp:52: DB::abortOnFailedAssertion(String const&, void* const*, unsigned long, unsigned long) @ 0x000000001c8bfd60
 [ 5596 ] {} <Fatal> BaseDaemon: 7. ./ci/tmp/build/./src/Common/Exception.cpp:58: ? @ 0x000000001c8c02f8
 [ 5596 ] {} <Fatal> BaseDaemon: 8. ./ci/tmp/build/./src/IO/ReadBuffer.cpp:90: DB::ReadBuffer::next() @ 0x000000001ca96e98
 [ 5596 ] {} <Fatal> BaseDaemon: 9.0. inlined from ./src/IO/ReadBuffer.h:81: DB::ReadBuffer::eof()
 [ 5596 ] {} <Fatal> BaseDaemon: 9.1. inlined from ./src/IO/ReadBuffer.h:158: DB::ReadBuffer::read(char*, unsigned long)
 [ 5596 ] {} <Fatal> BaseDaemon: 9. ./ci/tmp/build/./src/IO/StdStreamBufFromReadBuffer.cpp:51: DB::StdStreamBufFromReadBuffer::xsgetn(char*, long) @ 0x00000000267f1fc0
 [ 5596 ] {} <Fatal> BaseDaemon: 10.0. inlined from ./contrib/llvm-project/libcxx/include/streambuf:198: std::basic_streambuf<char, std::char_traits<char>>::sgetn[abi:ne190107](char*, long)
 [ 5596 ] {} <Fatal> BaseDaemon: 10. ./contrib/llvm-project/libcxx/include/istream:897: std::basic_istream<char, std::char_traits<char>>::read(char*, long) @ 0x0000000041aa0794
 [ 5596 ] {} <Fatal> BaseDaemon: 11. Aws::Utils::Crypto::CRTHash::Calculate(std::basic_istream<char, std::char_traits<char>>&) @ 0x000000003dca9590

Cherry pick #88668 to 25.8: wrap `~PooledConnection` in try-catch

Cherry pick #88668 to 25.9: wrap `~PooledConnection` in try-catch

CheSema · 2025-10-27T15:25:08Z

src/Common/HTTPConnectionPool.cpp

        ~PooledConnection() override
        {
-            if (bool(response_stream))
+            try


I kind of against to do

try {} catch (...) {}

in destructors where it is not necessary.

Here is it is not necessary after you made a proper fix in HTTPChunkedStreamBuf::isComplete.

Backport #88668 to 25.8: wrap `~PooledConnection` in try-catch

Backport #88668 to 25.9: wrap `~PooledConnection` in try-catch

…tion-dtor wrap `~PooledConnection` in try-catch

25.3.8 Backport of ClickHouse#88668: make `HTTPChunkedStreamBuf::isComplete` noexcept

wrap ~PooledConnection in try-catch

e38bf79

clickhouse-gh bot added pr-critical-bugfix pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-cloud labels Oct 16, 2025

CheSema self-assigned this Oct 16, 2025

alexey-milovidov reviewed Oct 16, 2025

View reviewed changes

src/Common/HTTPConnectionPool.cpp Outdated Show resolved Hide resolved

alexey-milovidov requested changes Oct 16, 2025

View reviewed changes

alexey-milovidov self-assigned this Oct 16, 2025

CheSema removed their assignment Oct 16, 2025

better

d14b109

fix typo: 02537_distributed_loosing_files_after_exception -> 02537_di…

655054a

…stributed_losing_files_after_exception

mstetsyuk requested a review from alexey-milovidov October 16, 2025 17:23

alexey-milovidov approved these changes Oct 16, 2025

View reviewed changes

mstetsyuk added this pull request to the merge queue Oct 17, 2025

Merged via the queue into master with commit b45b02d Oct 17, 2025
118 of 123 checks passed

mstetsyuk deleted the wrap-PooledConnection-dtor branch October 17, 2025 10:07

robot-clickhouse-ci-2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Oct 17, 2025

robot-ch-test-poll added pr-backports-created-cloud deprecated label, NOOP pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR labels Oct 17, 2025

This was referenced Oct 17, 2025

Cherry pick #88668 to 25.3: wrap ~PooledConnection in try-catch #88729

Closed

Cherry pick #88668 to 25.7: wrap ~PooledConnection in try-catch #88730

Closed

Cherry pick #88668 to 25.8: wrap ~PooledConnection in try-catch #88731

Merged

robot-ch-test-poll2 added a commit that referenced this pull request Oct 17, 2025

Merge pull request #88731 from ClickHouse/cherrypick/25.8/88668

47e8b72

Cherry pick #88668 to 25.8: wrap `~PooledConnection` in try-catch

robot-clickhouse added a commit that referenced this pull request Oct 17, 2025

Backport #88668 to 25.8: wrap ~PooledConnection in try-catch

bfa674c

This was referenced Oct 17, 2025

Backport #88668 to 25.8: wrap ~PooledConnection in try-catch #88732

Merged

Cherry pick #88668 to 25.9: wrap ~PooledConnection in try-catch #88733

Merged

robot-ch-test-poll2 added a commit that referenced this pull request Oct 17, 2025

Merge pull request #88733 from ClickHouse/cherrypick/25.9/88668

0818792

Cherry pick #88668 to 25.9: wrap `~PooledConnection` in try-catch

robot-clickhouse added a commit that referenced this pull request Oct 17, 2025

Backport #88668 to 25.9: wrap ~PooledConnection in try-catch

cf33ae4

robot-ch-test-poll2 mentioned this pull request Oct 17, 2025

Backport #88668 to 25.9: wrap ~PooledConnection in try-catch #88734

Merged

den-crane mentioned this pull request Oct 17, 2025

Crash report: segfault with ReplicatedMergeTree table #87465

Closed

CheSema reviewed Oct 27, 2025

View reviewed changes

mstetsyuk added a commit that referenced this pull request Oct 27, 2025

Merge pull request #88732 from ClickHouse/backport/25.8/88668

fd842b2

Backport #88668 to 25.8: wrap `~PooledConnection` in try-catch

mstetsyuk added a commit that referenced this pull request Oct 27, 2025

Merge pull request #88734 from ClickHouse/backport/25.9/88668

97ab9c8

Backport #88668 to 25.9: wrap `~PooledConnection` in try-catch

mstetsyuk changed the title ~~wrap ~PooledConnection in try-catch~~ make HTTPChunkedStreamBuf::isComplete noexcept Oct 27, 2025

robot-ch-test-poll4 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Oct 27, 2025

zvonand pushed a commit to Altinity/ClickHouse that referenced this pull request Oct 30, 2025

Merge pull request ClickHouse#88668 from ClickHouse/wrap-PooledConnec…

541eb68

…tion-dtor wrap `~PooledConnection` in try-catch

zvonand mentioned this pull request Oct 30, 2025

25.3.7 Backport of #88668: make HTTPChunkedStreamBuf::isComplete noexcept Altinity/ClickHouse#1114

Closed

25 tasks

zvonand pushed a commit to Altinity/ClickHouse that referenced this pull request Oct 31, 2025

Merge pull request ClickHouse#88668 from ClickHouse/wrap-PooledConnec…

c74e56b

…tion-dtor wrap `~PooledConnection` in try-catch

zvonand mentioned this pull request Oct 31, 2025

25.3.8 Backport of #88668: make HTTPChunkedStreamBuf::isComplete noexcept Altinity/ClickHouse#1117

Merged

25 tasks

zvonand added a commit to Altinity/ClickHouse that referenced this pull request Nov 1, 2025

Merge pull request #1117 from Altinity/backport/25.3.8/88668

f36ec92

25.3.8 Backport of ClickHouse#88668: make `HTTPChunkedStreamBuf::isComplete` noexcept

den-crane mentioned this pull request Nov 12, 2025

SIGABRT in ReadBufferFromHTTP destructor during ReplicatedMergeTree part fetching #89941

Closed

rienath mentioned this pull request Nov 27, 2025

remove try/catch from ~PooledConnection #89090

Merged

den-crane mentioned this pull request Dec 10, 2025

<Fatal> BaseDaemon: (version 25.7.2.54 (official build), build id: EBBA7923EC09CCFEAF7BA68169E7295C55E66D72 #91839

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make `HTTPChunkedStreamBuf::isComplete` noexcept#88668

make `HTTPChunkedStreamBuf::isComplete` noexcept#88668
mstetsyuk merged 3 commits intomasterfrom
wrap-PooledConnection-dtor

mstetsyuk commented Oct 16, 2025 •

edited

Loading

Uh oh!

clickhouse-gh bot commented Oct 16, 2025 •

edited

Loading

Uh oh!

CheSema commented Oct 16, 2025

Uh oh!

Uh oh!

alexey-milovidov left a comment

Uh oh!

alexey-milovidov commented Oct 16, 2025

Uh oh!

alexey-milovidov commented Oct 16, 2025

Uh oh!

mstetsyuk commented Oct 16, 2025

Uh oh!

alexey-milovidov left a comment

Uh oh!

alexey-milovidov commented Oct 16, 2025

Uh oh!

Uh oh!

CheSema Oct 27, 2025

Uh oh!

mstetsyuk Oct 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

mstetsyuk commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Documentation entry for user-facing changes

Uh oh!

clickhouse-gh bot commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CheSema commented Oct 16, 2025

Uh oh!

Uh oh!

alexey-milovidov left a comment

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov commented Oct 16, 2025

Uh oh!

alexey-milovidov commented Oct 16, 2025

Uh oh!

mstetsyuk commented Oct 16, 2025

Uh oh!

alexey-milovidov left a comment

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov commented Oct 16, 2025

Uh oh!

Uh oh!

CheSema Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

mstetsyuk Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mstetsyuk commented Oct 16, 2025 •

edited

Loading

clickhouse-gh bot commented Oct 16, 2025 •

edited

Loading