Skip to content

Fix clickhouse-data: make NonBlockingPipedOutputStream.close() idempotent - #3091

Open
jjj-n wants to merge 1 commit into
ClickHouse:mainfrom
jjj-n:codex/fix-nonblocking-piped-output-stream-close-race
Open

Fix clickhouse-data: make NonBlockingPipedOutputStream.close() idempotent#3091
jjj-n wants to merge 1 commit into
ClickHouse:mainfrom
jjj-n:codex/fix-nonblocking-piped-output-stream-close-race

Conversation

@jjj-n

@jjj-n jjj-n commented Sep 1, 2026

Copy link
Copy Markdown

Summary

Make NonBlockingPipedOutputStream.close() atomically claim the close path before flushing its pending buffer. Previously, two concurrent callers could both pass the volatile closed check, mutate and enqueue the same ByteBuffer, enqueue the end-of-stream marker twice, and run the post-close action twice. If the second caller changed the already-queued buffer's limit to zero, the reader observed a clean EOF and silently lost the pending payload.

Exactly one caller now flushes pending data, enqueues the end-of-stream marker, and runs the post-close action. Concurrent or repeated close() calls return immediately. The implementation follows the AtomicBoolean pattern already used by BlockingPipedOutputStream, without holding the instance monitor across the close path or a potentially blocking post-close action.

Closes #3057

Compatibility

There is no public API, configuration, default, output-format, serialization, or exception-type change. A single close() keeps its existing behavior. Only a redundant concurrent close changes from racing with the first caller to returning without touching the buffer or queue. docs/features.md is not affected because this change is confined to clickhouse-data.

Test

Added testConcurrentClose, which holds the first close at the pending-buffer offer, invokes a second close concurrently, and verifies that the second caller returns without waiting, the post-close action runs exactly once, the pending bytes are preserved, and the reader sees EOF afterward.

  • Before the production fix, the new test failed with expected [1] but found [2] for the post-close action count.
  • mvn -pl clickhouse-data -Dtest=NonBlockingPipedOutputStreamTest test: 6 tests passed.
  • mvn -pl clickhouse-data test: 1,668 tests passed; 113 skipped by the existing suite configuration.
  • mvn -Dj8 -pl clickhouse-data -Dtest=NonBlockingPipedOutputStreamTest test: Java 8 source/target compilation succeeded and 6 tests passed.

Checklist

Claim the close path atomically so only one concurrent caller flushes the pending buffer, enqueues EOF, and runs the post-close action.

Fixes: ClickHouse#3057
@CLAassistant

CLAassistant commented Sep 1, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NonBlockingPipedOutputStream.close() is not idempotent: concurrent close silently drops the pending buffer

3 participants