Split public and internal drainStream and cover with tests#578
Split public and internal drainStream and cover with tests#578peter-leonov-ch merged 14 commits intomainfrom
drainStream and cover with tests#578Conversation
peter-leonov-ch
force-pushed
the
always_drain_stream_2
branch
from
f769ca8 to
0507b34
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
peter-leonov-ch
changed the title
drainStream with tests
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive test coverage for the drainStream functionality and splits it into two separate functions: a public drainStream API and an internal drainStreamInternal function with enhanced logging. The changes address race conditions where streams may already be in a terminal state (errored, ended, or closed) before draining begins.
Changes:
- Split
drainStreaminto public API anddrainStreamInternalwith logging - Added early return checks for streams that are already errored, ended, or closed to prevent race conditions
- Added comprehensive unit tests for both functions covering all major stream states
- Updated internal code to use
drainStreamInternalinstead ofdrainStream
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/client-node/src/connection/stream.ts | Introduced new public drainStream function and refactored existing one to drainStreamInternal with added logging and early state checks |
| packages/client-node/src/connection/node_base_connection.ts | Updated all internal calls to use drainStreamInternal instead of drainStream; minor code organization improvements |
| packages/client-node/tests/unit/node_stream.test.ts | Added comprehensive unit tests for the public drainStream API |
| packages/client-node/tests/unit/node_stream_internal.test.ts | Added comprehensive unit tests for the internal drainStreamInternal function |
| packages/client-node/tests/integration/node_exec.test.ts | Updated integration tests to use drainStreamInternal for internal test utilities |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/client-node/__tests__/unit/node_stream_internal.test.ts
Outdated
Show resolved
Hide resolved
peter-leonov-ch
force-pushed
the
always_drain_stream_2
branch
from
838eaa9 to
d7bd883
Compare
peter-leonov-ch
changed the title
drainStream with testsdrainStream and cover with tests
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/client-node/__tests__/unit/node_stream_internal.test.ts
Outdated
Show resolved
Hide resolved
| * will result in closing the underlying socket, and negate the KeepAlive feature benefits. | ||
| * See https://github.com/ClickHouse/clickhouse-js/pull/203 */ | ||
| export async function drainStream( | ||
| export async function drainStream(stream: Stream.Readable): Promise<void> { |
There was a problem hiding this comment.
what is the rationale to make the method public?
There was a problem hiding this comment.
@slvrtrn can you help us out here? I see sleep is also public in the common module?
Maybe we cut the public ones asap until we know what's required, this should help reduce the API surface to support.
|
@copilot review risks this PR can introduce. |
|
@peter-leonov-ch I've opened a new pull request, #582, to work on those changes. Once the pull request is ready, I'll request review from you. |
well, internally :)
peter-leonov-ch
force-pushed
the
always_drain_stream_2
branch
from
6e90efb to
b976d08
Compare
Summary
This is a more conservative version of:
Here we keep the iterator approach for future.
Also, splitting the public API
drainStream()from the newdrainStreamInternal()internal one.