Skip to content

Check for cancelled ReadBuffer in TCPHandler on client disconnection#100585

Closed
Avogar wants to merge 1 commit intomasterfrom
fix-read-from-cancelled-buf
Closed

Check for cancelled ReadBuffer in TCPHandler on client disconnection#100585
Avogar wants to merge 1 commit intomasterfrom
fix-read-from-cancelled-buf

Conversation

@Avogar
Copy link
Copy Markdown
Member

@Avogar Avogar commented Mar 24, 2026 •
edited
Loading

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):

Check for cancelled ReadBuffer in TCPHandler on client disconnection to avoid reading from bad ReadBuffer. Closes #100581

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@Avogar
Copy link
Copy Markdown
Member Author

Avogar commented Mar 24, 2026

Unfortunately I coudn't create a test, as it requires triggering a network disconnection at a particular place in the code.

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 24, 2026 •
edited
Loading

Workflow [PR], commit [091dfcb]

Summary: ❌

job_name test_name status info comment
Finish Workflow failure
python3 ./ci/jobs/scripts/workflow_hooks/new_tests_check.py failure

AI Review

Summary

This PR adds an in->isCanceled() guard to TCPHandler::runImpl before reading the next packet, so disconnected/cancelled client input streams are closed early instead of proceeding to read from a cancelled ReadBuffer. The change is small, targeted, and aligns with existing cancellation handling later in the same file; I did not find correctness, safety, concurrency, or performance issues in the patch.

ClickHouse Rules
Item Status Notes
Deletion logging ➖
Serialization versioning ➖
Core-area scrutiny ✅
No test removal ✅
Experimental gate ➖
No magic constants ✅
Backward compatibility ✅
SettingsChangesHistory.cpp ➖
PR metadata quality ✅
Safe rollout ✅
Compilation time ✅
Final Verdict
  • Status: ✅ Approve

@clickhouse-gh clickhouse-gh bot added the pr-bugfix Pull request with bugfix, not backported by default label Mar 24, 2026
@kssenii
Copy link
Copy Markdown
Member

kssenii commented Mar 24, 2026

Unfortunately I coudn't create a test, as it requires triggering a network disconnection at a particular place in the code.

Use a failpoint? Unless this case does not warrant adding extra failpoint

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 24, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 84.10% 84.10% +0.00%
Functions 24.50% 24.40% -0.10%
Branches 76.60% 76.60% +0.00%

PR changed lines: PR changed-lines coverage: 100.00% (5/5, 0 noise lines excluded)
Diff coverage report
Uncovered code

@Algunenano Algunenano mentioned this pull request Mar 25, 2026
Merged
1 task
@nikitamikhaylov
Copy link
Copy Markdown
Member

nikitamikhaylov commented Mar 25, 2026

A little bit different implementation #100536

@Avogar
Copy link
Copy Markdown
Member Author

Avogar commented Mar 25, 2026

That one looks better, let's close this

@Avogar Avogar closed this Mar 25, 2026
@Avogar Avogar mentioned this pull request Mar 25, 2026
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

pr-bugfix Pull request with bugfix, not backported by default

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Logical error: 'ReadBuffer is canceled. Can't read from it.' (STID: 2508-2913)

3 participants

@Avogar @kssenii @nikitamikhaylov