Fix HTTP compressors finalization by yakov-olkhovskiy · Pull Request #58846 · ClickHouse/ClickHouse

yakov-olkhovskiy · 2024-01-16T00:34:01Z

Changelog category (leave one):

Bug Fix (user-visible misbehavior in an official stable release)

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

Fix HTTP compressors. Follow-up #58475

Manifests as error when HTTP request includes compress=1&enable_http_compression=1 along with HTTP header Accept-Encoding:

curl -vv -d 'SELECT 1,2,3,42' -H 'Accept-Encoding: bz2' -X POST 'http://localhost:8123/?compress=1&enable_http_compression=1'

2024.01.16 00:09:02.684543 [ 1820868 ] {f7ca0e8c-6df8-4041-8277-1633b1c497bc} <Fatal> : Logical error: 'false && "WriteBuffer is not finalized in destructor."'.
2024.01.16 00:09:02.685075 [ 1820867 ] {} <Trace> BaseDaemon: Received signal 6
2024.01.16 00:09:02.685770 [ 1822771 ] {} <Fatal> BaseDaemon: ########## Short fault info ############
2024.01.16 00:09:02.685889 [ 1822771 ] {} <Fatal> BaseDaemon: (version 24.1.1.1, build id: 340551B33BEA709A0808F0761D6096F813A3209C, git hash: 61889faabd72dcf6199389df581e2
2d0c4cb8a8f) (from thread 1820868) Received signal 6
2024.01.16 00:09:02.685980 [ 1822771 ] {} <Fatal> BaseDaemon: Signal description: Aborted
2024.01.16 00:09:02.686026 [ 1822771 ] {} <Fatal> BaseDaemon:
2024.01.16 00:09:02.686129 [ 1822771 ] {} <Fatal> BaseDaemon: Stack trace: 0x00007f1c2ee969fc 0x00007f1c2ee42476 0x00007f1c2ee287f3 0x000000001390d55f 0x00000000139c8e78 0x
00000000139ca2d4 0x0000000016dc56d4 0x0000000016dd4174 0x0000000016dd4199 0x0000000016dc4894 0x000000001dc9578a 0x000000000aa24bd4 0x000000000aa24b79 0x0000000013d0aeec 0x0
00000001ddb7151 0x000000001ddab074 0x000000001de5abc7 0x000000002328bf59 0x000000002328c79c 0x0000000023461594 0x000000002345e33a 0x000000002345d03e 0x00007f1c2ee94ac3 0x00
007f1c2ef26850
2024.01.16 00:09:02.686227 [ 1822771 ] {} <Fatal> BaseDaemon: ########################################
2024.01.16 00:09:02.686299 [ 1822771 ] {} <Fatal> BaseDaemon: (version 24.1.1.1, build id: 340551B33BEA709A0808F0761D6096F813A3209C, git hash: 61889faabd72dcf6199389df581e2
2d0c4cb8a8f) (from thread 1820868) (query_id: f7ca0e8c-6df8-4041-8277-1633b1c497bc) (query: SELECT 1,2,3,42) Received signal Aborted (6)
2024.01.16 00:09:02.686352 [ 1822771 ] {} <Fatal> BaseDaemon:
2024.01.16 00:09:02.686428 [ 1822771 ] {} <Fatal> BaseDaemon: Stack trace: 0x00007f1c2ee969fc 0x00007f1c2ee42476 0x00007f1c2ee287f3 0x000000001390d55f 0x00000000139c8e78 0x
00000000139ca2d4 0x0000000016dc56d4 0x0000000016dd4174 0x0000000016dd4199 0x0000000016dc4894 0x000000001dc9578a 0x000000000aa24bd4 0x000000000aa24b79 0x0000000013d0aeec 0x0
00000001ddb7151 0x000000001ddab074 0x000000001de5abc7 0x000000002328bf59 0x000000002328c79c 0x0000000023461594 0x000000002345e33a 0x000000002345d03e 0x00007f1c2ee94ac3 0x00
007f1c2ef26850
2024.01.16 00:09:02.686497 [ 1822771 ] {} <Fatal> BaseDaemon: 4. ? @ 0x00007f1c2ee969fc in ?
2024.01.16 00:09:02.686551 [ 1822771 ] {} <Fatal> BaseDaemon: 5. ? @ 0x00007f1c2ee42476 in ?
2024.01.16 00:09:02.686605 [ 1822771 ] {} <Fatal> BaseDaemon: 6. ? @ 0x00007f1c2ee287f3 in ?
2024.01.16 00:09:02.785902 [ 1822771 ] {} <Fatal> BaseDaemon: 7. /home/ubuntu/ClickHouse-insjoin/src/Common/Exception.cpp:0: DB::abortOnFailedAssertion(String const&) @ 0x0
00000001390d55f in /home/ubuntu/ClickHouse-insjoin/build_debug/programs/clickhouse
2024.01.16 00:09:02.814179 [ 1822771 ] {} <Fatal> BaseDaemon: 8. /home/ubuntu/ClickHouse-insjoin/src/IO/WriteBuffer.cpp:28: DB::WriteBuffer::~WriteBuffer() @ 0x00000000139c
8e78 in /home/ubuntu/ClickHouse-insjoin/build_debug/programs/clickhouse
2024.01.16 00:09:02.849031 [ 1822771 ] {} <Fatal> BaseDaemon: 9. /home/ubuntu/ClickHouse-insjoin/src/IO/BufferWithOwnMemory.h:150: DB::BufferWithOwnMemory<DB::WriteBuffer>:
:~BufferWithOwnMemory() @ 0x00000000139ca2d4 in /home/ubuntu/ClickHouse-insjoin/build_debug/programs/clickhouse
2024.01.16 00:09:02.897046 [ 1822771 ] {} <Fatal> BaseDaemon: 10. /home/ubuntu/ClickHouse-insjoin/src/IO/WriteBufferDecorator.h:18: DB::WriteBufferDecorator<DB::BufferWithO
wnMemory<DB::WriteBuffer>>::~WriteBufferDecorator() @ 0x0000000016dc56d4 in /home/ubuntu/ClickHouse-insjoin/build_debug/programs/clickhouse
2024.01.16 00:09:02.922463 [ 1822771 ] {} <Fatal> BaseDaemon: 11. /home/ubuntu/ClickHouse-insjoin/src/IO/Bzip2WriteBuffer.cpp:36: DB::Bzip2WriteBuffer::~Bzip2WriteBuffer()
@ 0x0000000016dd4174 in /home/ubuntu/ClickHouse-insjoin/build_debug/programs/clickhouse
2024.01.16 00:09:02.947788 [ 1822771 ] {} <Fatal> BaseDaemon: 12. /home/ubuntu/ClickHouse-insjoin/src/IO/Bzip2WriteBuffer.cpp:36: DB::Bzip2WriteBuffer::~Bzip2WriteBuffer()
@ 0x0000000016dd4199 in /home/ubuntu/ClickHouse-insjoin/build_debug/programs/clickhouse
2024.01.16 00:09:02.994841 [ 1822771 ] {} <Fatal> BaseDaemon: 13. /home/ubuntu/ClickHouse-insjoin/contrib/llvm-project/libcxx/include/__memory/unique_ptr.h:49: std::default
_delete<DB::WriteBuffer>::operator()[abi:v15000](DB::WriteBuffer*) const @ 0x0000000016dc4894 in /home/ubuntu/ClickHouse-insjoin/build_debug/programs/clickhouse
2024.01.16 00:09:03.003885 [ 1821503 ] {} <Trace> AsynchronousMetrics: MemoryTracking: was 588.97 MiB, peak 593.00 MiB, free memory in arenas 131.96 MiB, will set to 726.07
 MiB (RSS), difference: 137.09 MiB
2024.01.16 00:09:03.633403 [ 1822771 ] {} <Fatal> BaseDaemon: 14. /home/ubuntu/ClickHouse-insjoin/contrib/llvm-project/libcxx/include/__memory/shared_ptr.h:263: std::__shar
ed_ptr_pointer<DB::WriteBuffer*, std::default_delete<DB::WriteBuffer>, std::allocator<DB::WriteBuffer>>::__on_zero_shared() @ 0x000000001dc9578a in /home/ubuntu/ClickHouse-
insjoin/build_debug/programs/clickhouse
2024.01.16 00:09:03.722939 [ 1822771 ] {} <Fatal> BaseDaemon: 15. /home/ubuntu/ClickHouse-insjoin/contrib/llvm-project/libcxx/include/__memory/shared_ptr.h:175: std::__shar
ed_count::__release_shared[abi:v15000]() @ 0x000000000aa24bd4 in /home/ubuntu/ClickHouse-insjoin/build_debug/programs/clickhouse
2024.01.16 00:09:03.812244 [ 1822771 ] {} <Fatal> BaseDaemon: 16. /home/ubuntu/ClickHouse-insjoin/contrib/llvm-project/libcxx/include/__memory/shared_ptr.h:215: std::__shar
ed_weak_count::__release_shared[abi:v15000]() @ 0x000000000aa24b79 in /home/ubuntu/ClickHouse-insjoin/build_debug/programs/clickhouse
2024.01.16 00:09:03.908493 [ 1822771 ] {} <Fatal> BaseDaemon: 17. /home/ubuntu/ClickHouse-insjoin/contrib/llvm-project/libcxx/include/__memory/shared_ptr.h:703: std::shared
_ptr<DB::WriteBuffer>::~shared_ptr[abi:v15000]() @ 0x0000000013d0aeec in /home/ubuntu/ClickHouse-insjoin/build_debug/programs/clickhouse
2024.01.16 00:09:04.003828 [ 1821503 ] {} <Trace> AsynchronousMetrics: MemoryTracking: was 726.07 MiB, peak 726.07 MiB, free memory in arenas 132.00 MiB, will set to 827.82
 MiB (RSS), difference: 101.75 MiB
2024.01.16 00:09:04.124539 [ 1822771 ] {} <Fatal> BaseDaemon: 18. /home/ubuntu/ClickHouse-insjoin/src/Server/HTTPHandler.h:47: DB::HTTPHandler::Output::~Output() @ 0x000000
001ddb7151 in /home/ubuntu/ClickHouse-insjoin/build_debug/programs/clickhouse
2024.01.16 00:09:04.326570 [ 1822771 ] {} <Fatal> BaseDaemon: 19. /home/ubuntu/ClickHouse-insjoin/src/Server/HTTPHandler.cpp:1131: DB::HTTPHandler::handleRequest(DB::HTTPSe
rverRequest&, DB::HTTPServerResponse&, StrongTypedef<unsigned long, ProfileEvents::EventTag> const&) @ 0x000000001ddab074 in /home/ubuntu/ClickHouse-insjoin/build_debug/pro
grams/clickhouse
2024.01.16 00:09:04.352564 [ 1822771 ] {} <Fatal> BaseDaemon: 20. /home/ubuntu/ClickHouse-insjoin/src/Server/HTTP/HTTPServerConnection.cpp:70: DB::HTTPServerConnection::run
() @ 0x000000001de5abc7 in /home/ubuntu/ClickHouse-insjoin/build_debug/programs/clickhouse
2024.01.16 00:09:04.375774 [ 1822771 ] {} <Fatal> BaseDaemon: 21. /home/ubuntu/ClickHouse-insjoin/base/poco/Net/src/TCPServerConnection.cpp:43: Poco::Net::TCPServerConnecti
on::start() @ 0x000000002328bf59 in /home/ubuntu/ClickHouse-insjoin/build_debug/programs/clickhouse
2024.01.16 00:09:04.405781 [ 1822771 ] {} <Fatal> BaseDaemon: 22. /home/ubuntu/ClickHouse-insjoin/base/poco/Net/src/TCPServerDispatcher.cpp:115: Poco::Net::TCPServerDispatc
her::run() @ 0x000000002328c79c in /home/ubuntu/ClickHouse-insjoin/build_debug/programs/clickhouse
2024.01.16 00:09:04.438307 [ 1822771 ] {} <Fatal> BaseDaemon: 23. /home/ubuntu/ClickHouse-insjoin/base/poco/Foundation/src/ThreadPool.cpp:188: Poco::PooledThread::run() @ 0
x0000000023461594 in /home/ubuntu/ClickHouse-insjoin/build_debug/programs/clickhouse
2024.01.16 00:09:04.468876 [ 1822771 ] {} <Fatal> BaseDaemon: 24. /home/ubuntu/ClickHouse-insjoin/base/poco/Foundation/src/Thread.cpp:46: Poco::(anonymous namespace)::Runna
bleHolder::run() @ 0x000000002345e33a in /home/ubuntu/ClickHouse-insjoin/build_debug/programs/clickhouse
2024.01.16 00:09:04.498020 [ 1822771 ] {} <Fatal> BaseDaemon: 25. /home/ubuntu/ClickHouse-insjoin/base/poco/Foundation/src/Thread_POSIX.cpp:335: Poco::ThreadImpl::runnableE
ntry(void*) @ 0x000000002345d03e in /home/ubuntu/ClickHouse-insjoin/build_debug/programs/clickhouse
2024.01.16 00:09:04.498109 [ 1822771 ] {} <Fatal> BaseDaemon: 26. ? @ 0x00007f1c2ee94ac3 in ?
2024.01.16 00:09:04.498179 [ 1822771 ] {} <Fatal> BaseDaemon: 27. ? @ 0x00007f1c2ef26850 in ?
2024.01.16 00:09:04.498252 [ 1822771 ] {} <Fatal> BaseDaemon: Integrity check of the executable skipped because the reference checksum could not be read.
2024.01.16 00:09:04.498322 [ 1822771 ] {} <Information> SentryWriter: Not sending crash report
2024.01.16 00:09:04.498397 [ 1822771 ] {} <Fatal> BaseDaemon: This ClickHouse version is not official and should be upgraded to the official build.

robot-ch-test-poll2 · 2024-01-16T00:37:21Z

This is an automated comment for commit d5d060a 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

Check name	Description	Status
AST fuzzer	Runs randomly generated queries to catch program errors. The build type is optionally given in parenthesis. If it fails, ask a maintainer for help	✅ success
ClickBench	Runs [ClickBench](https://github.com/ClickHouse/ClickBench/) with instant-attach table	✅ success
ClickHouse build check	Builds ClickHouse in various configurations for use in further steps. You have to fix the builds that fail. Build logs often has enough information to fix the error, but you might have to reproduce the failure locally. The cmake options can be found in the build log, grepping for cmake. Use these options and follow the general build process	✅ success
Compatibility check	Checks that clickhouse binary runs on distributions with old libc versions. If it fails, ask a maintainer for help	✅ success
Docker image for servers	The check to build and optionally push the mentioned image to docker hub	✅ success
Docs Check	Builds and tests the documentation	✅ success
Fast test	Normally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here	✅ success
Flaky tests	Checks if new added or modified tests are flaky by running them repeatedly, in parallel, with more randomization. Functional tests are run 100 times with address sanitizer, and additional randomization of thread scheduling. Integrational tests are run up to 10 times. If at least once a new test has failed, or was too long, this check will be red. We don't allow flaky tests, read the doc	✅ success
Install packages	Checks that the built packages are installable in a clear environment	✅ success
Performance Comparison	Measure changes in query performance. The performance test report is described in detail here. In square brackets are the optional part/total tests	✅ success
SQLTest	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
SQLancer	Fuzzing tests that detect logical bugs with SQLancer tool	✅ success
Stateful tests	Runs stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Stateless tests	Runs stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Stress test	Runs stateless functional tests concurrently from several clients to detect concurrency-related errors	✅ success
Style Check	Runs a set of checks to keep the code style clean. If some of tests failed, see the related log from the report	✅ success
Unit tests	Runs the unit tests for different release types	✅ success

Check name	Description	Status
Bugfix validate check	Checks that either a new test (functional or integration) or there some changed tests that fail with the binary built on master branch	❌ error
CI running	A meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR	⏳ pending
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	❌ failure
Mergeable Check	Checks if all other necessary checks are successful	❌ failure
Upgrade check	Runs stress tests on server version from last release and then tries to upgrade it to the version from the PR. It checks if the new server can successfully startup without any errors, crashes or sanitizer asserts	❌ failure

alexey-milovidov · 2024-01-16T07:20:36Z

Please add a test.

PS. It does not seem right to call finalize in destructor. The ideal flow is when you call finalize explicitly before destructing, or if you didn't, the destructor ignores the data.

But this change is also ok for me.

antonio2368 · 2024-01-16T11:23:27Z

This is reverting work done by @CheSema in #50395

novikd · 2024-01-16T11:25:54Z

I agree with @antonio2368 I think it's better to find where buffers should call finalize and do it explicitly.

CheSema · 2024-01-16T13:04:52Z

if you didn't, the destructor ignores the data.

You are understand right what would happen. That could lead to data loss.

But this change is also ok for me.

I don't think that it could ever be ok.

yakov-olkhovskiy · 2024-01-16T14:05:57Z

@CheSema I would argue that it's not always practical to not utilize destructor for the exact purpose destructor exists - for example if we use temporary object or some cases of exception processing. Data loss is rather signifier of incorrect objects' ownership, lack of proper objects' initialization and similar issues. Though I agree that in this particular case it's better to add explicit finalization, overall if we have a case when finalization in destructor leads to data loss or data corruption - there is a deeper problem worth of investigation and proper fix.

CheSema · 2024-01-16T15:24:03Z

@CheSema I would argue that it's not always practical to not utilize destructor for the exact purpose destructor exists - for example if we use temporary object or some cases of exception processing. Data loss is rather signifier of incorrect objects' ownership, lack of proper objects' initialization and similar issues. Though I agree that in this particular case it's better to add explicit finalization, overall if we have a case when finalization in destructor leads to data loss or data corruption - there is a deeper problem worth of investigation and proper fix.

I don't wont to argue here at all.
Using d-tor is OK. Mute the error which could be thrown at finalization -- not OK. There is nothing to argue about.

There is a deep problem -- we use buffers API to write a file. All such tasks/conversations are the consequences of that problem.
It is better to put some efforts to solve this problem. I have tried. But it really complex and time consuming. I haven't solved it completely. I would be glad to have a hand here.

yakov-olkhovskiy · 2024-01-16T17:02:54Z

Mute the error which could be thrown at finalization -- not OK.

It's not muted - it's reported in catch. I understand the concern of having finalize in destructor can encourage ppl to neglect explicit finalization, but maybe it shouldn't outweigh the usefulness of this...

yakov-olkhovskiy · 2024-01-17T20:25:49Z

@CheSema @novikd could you please expedite review? so I could merge it as CI finish - ppl need it ASAP

fix compressors finalization

fcd4297

robot-ch-test-poll2 added the pr-bugfix Pull request with bugfix, not backported by default label Jan 16, 2024

novikd self-assigned this Jan 16, 2024

yakov-olkhovskiy marked this pull request as draft January 16, 2024 14:08

explicit finalization for chained buffer

73f1ed3

yakov-olkhovskiy added 2 commits January 16, 2024 22:15

separate explicit finalization for buffers

92d4256

test is added

1bda1bc

yakov-olkhovskiy marked this pull request as ready for review January 16, 2024 22:59

yakov-olkhovskiy added 7 commits January 16, 2024 23:40

fix test

a9e12d7

fix test

5da897a

refactoring bzip2 buffer and fix possible data loss in internal buffer

e06e469

style fix

e6b3f69

fix test

1d9ac13

fix test

7706840

fix memory leak in bzip2

d5d060a

yakov-olkhovskiy requested a review from CheSema January 17, 2024 20:26

alexey-milovidov approved these changes Jan 17, 2024

View reviewed changes

alexey-milovidov self-assigned this Jan 17, 2024

yakov-olkhovskiy merged commit 490bb80 into master Jan 17, 2024

yakov-olkhovskiy deleted the fix-compress-destructor branch January 17, 2024 22:49

filimonov mentioned this pull request Jan 23, 2024

23.3: uncaught exception from WriteBufferFromHTTPServerResponse desctuctor & friends #59129

Closed

This was referenced Jun 20, 2024

Fix error due to output buffer not finalized correctly ByConity/ByConity#1720

Closed

Fix error due to output buffer not finalized correctly ByConity/ByConity#1724

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix HTTP compressors finalization#58846

Fix HTTP compressors finalization#58846
yakov-olkhovskiy merged 11 commits intomasterfrom
fix-compress-destructor

yakov-olkhovskiy commented Jan 16, 2024

Uh oh!

robot-ch-test-poll2 commented Jan 16, 2024 •

edited by robot-clickhouse-ci-2

Loading

Uh oh!

alexey-milovidov commented Jan 16, 2024 •

edited

Loading

Uh oh!

antonio2368 commented Jan 16, 2024

Uh oh!

novikd commented Jan 16, 2024

Uh oh!

CheSema commented Jan 16, 2024 •

edited

Loading

Uh oh!

yakov-olkhovskiy commented Jan 16, 2024

Uh oh!

CheSema commented Jan 16, 2024 •

edited

Loading

Uh oh!

yakov-olkhovskiy commented Jan 16, 2024 •

edited

Loading

Uh oh!

yakov-olkhovskiy commented Jan 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

yakov-olkhovskiy commented Jan 16, 2024

Changelog category (leave one):

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

Uh oh!

robot-ch-test-poll2 commented Jan 16, 2024 • edited by robot-clickhouse-ci-2 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexey-milovidov commented Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antonio2368 commented Jan 16, 2024

Uh oh!

novikd commented Jan 16, 2024

Uh oh!

CheSema commented Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yakov-olkhovskiy commented Jan 16, 2024

Uh oh!

CheSema commented Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yakov-olkhovskiy commented Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yakov-olkhovskiy commented Jan 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

robot-ch-test-poll2 commented Jan 16, 2024 •

edited by robot-clickhouse-ci-2

Loading

alexey-milovidov commented Jan 16, 2024 •

edited

Loading

CheSema commented Jan 16, 2024 •

edited

Loading

CheSema commented Jan 16, 2024 •

edited

Loading

yakov-olkhovskiy commented Jan 16, 2024 •

edited

Loading