Skip to content

Flush gzip buffer when cancelling http response buffer#89256

Merged
tavplubix merged 4 commits intomasterfrom
http_flush_gzip_on_error
Nov 6, 2025
Merged

Flush gzip buffer when cancelling http response buffer#89256
tavplubix merged 4 commits intomasterfrom
http_flush_gzip_on_error

Conversation

@tavplubix
Copy link
Copy Markdown
Member

@tavplubix tavplubix commented Oct 30, 2025

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 into CHANGELOG.md):

Flush buffers when sending an error in the middle of compressed stream in HTTP interface.

Details

See #75175 (comment)

The problem is that ZlibDeflatingWriteBuffer::nextImpl() doesn't necessarily flush the buffer, we need to finalize it.

@tavplubix tavplubix requested a review from CheSema October 30, 2025 19:04
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Oct 30, 2025

Workflow [PR], commit [fcac704]

Summary:

job_name test_name status info comment
Integration tests (amd_asan, old analyzer, 4/6) failure
test_scheduler_cpu_preemptive/test.py::test_downscaling[cpu-slot-preemption-timeout-1ms] FAIL cidb, flaky
test_merges_memory_limit/test.py::test_memory_limit_success FAIL cidb
test_backup_restore_on_cluster/test_huge_concurrent_restore.py::test_backup_restore_huge_cluster FAIL cidb
test_replicated_merge_tree_compatibility/test.py::test_replicated_merge_tree_defaults_compatibility FAIL cidb
OOM in dmesg FAIL cidb
Integration tests (amd_tsan, 3/6) failure
test_replication_without_zookeeper/test.py::test_startup_without_zookeeper FAIL cidb
Upgrade check (amd_debug) failure
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
Found signal in gdb.log FAIL cidb

@clickhouse-gh clickhouse-gh bot added the pr-bugfix Pull request with bugfix, not backported by default label Oct 30, 2025
@CheSema CheSema self-assigned this Oct 31, 2025
Add a tag to the HTTP flush test script.
@@ -0,0 +1,2 @@
zstd OK
Copy link
Copy Markdown
Contributor

@kavirajk kavirajk Nov 1, 2025

Choose a reason for hiding this comment

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

@tavplubix FYI: Currently it also broken on other compressions like brotli

for example

echo "select throwIf(number > 10000000) + number check from numbers(10000005) format CSV" | curl "http://127.0.0.1:8123/?query=&enable_http_compression=1" -H "Accept-Encoding: br" --compressed -v --data-binary @- | tail -c 1024

is the intention is to fix all the broken compression in this PR or just gzip?

(it would be nice to write tests for all the compression supported by CH server?)

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.

is the intention is to fix all the broken compression in this PR or just gzip?

all, it fixes brotli as well, I added it to the test

(it would be nice to write tests for all the compression supported by CH server?)

curl supports only deflate, gzip, br and zstd, so adding other compression methods to the test is not trivial, but I don't think it's necessary since the fix is generic

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @tavplubix. make sense 👍

@kavirajk
Copy link
Copy Markdown
Contributor

kavirajk commented Nov 6, 2025

@tavplubix can we merge this? failing tests looks unrelated?

@tavplubix tavplubix enabled auto-merge November 6, 2025 10:28
@tavplubix tavplubix added this pull request to the merge queue Nov 6, 2025
Merged via the queue into master with commit 7f2ba2f Nov 6, 2025
121 of 125 checks passed
@tavplubix tavplubix deleted the http_flush_gzip_on_error branch November 6, 2025 10:42
@robot-ch-test-poll3 robot-ch-test-poll3 added the pr-synced-to-cloud The PR is synced to the cloud repo label Nov 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-bugfix Pull request with bugfix, not backported by default pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants