Implement ByteReadChannel.readTo util - #5216
Conversation
WalkthroughAdds a new suspend function ByteReadChannel.readTo(sink: RawSink, limit: Long = Long.MAX_VALUE): Long and a bounded overload ByteReadChannel.copyTo(channel: ByteWriteChannel, limit: Long): Long; updates copy loop behavior to perform per-iteration flushes and error handling; adds tests for full, partial, and cancelled reads. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.kt📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-09-05T12:46:14.074ZApplied to files:
📚 Learning: 2025-11-25T09:38:19.393ZApplied to files:
📚 Learning: 2025-11-25T09:38:19.393ZApplied to files:
📚 Learning: 2025-09-05T12:47:49.016ZApplied to files:
🧬 Code graph analysis (1)ktor-io/common/src/io/ktor/utils/io/ByteReadChannelOperations.kt (5)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ktor-io/common/src/io/ktor/utils/io/ByteReadChannelOperations.kt (2)
186-203: Add KDoc documentation for this new public API.Per coding guidelines, all public APIs must be documented including parameters, return types, and exceptions. This function is missing documentation.
Also, unlike the unbounded
copyTo(channel: ByteWriteChannel)at line 179 which has afinally { channel.flush() }block, this function relies solely on in-loop flushes. If the loop exits normally (channel closed for read withremaining > 0), the last write was already flushed inside the loop, so this should be fine—but please verify this is intentional.Consider adding documentation similar to:
/** * Reads up to [limit] bytes from this channel and writes them to the [sink]. * * @param sink the destination to write bytes to * @param limit the maximum number of bytes to read * @return the number of bytes actually read and written */
205-213: Add KDoc documentation for this public API overload.This new overload should be documented to explain the bounded copy behavior, parameters, and return value.
Consider adding documentation:
/** * Copies up to [limit] bytes from this channel to the specified [channel]. * * @param channel the destination channel to copy bytes to * @param limit the maximum number of bytes to copy * @return the number of bytes actually copied */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ktor-io/common/src/io/ktor/utils/io/ByteReadChannelOperations.kt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.kt
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.kt: Follow Kotlin official style guide (https://kotlinlang.org/docs/coding-conventions.html)
Use star imports forio.ktor.*packages
Document all public APIs including parameters, return types, and exceptions
Mark internal APIs with@InternalAPIannotation
Run./gradlew lintKotlinand fix all linting issues before giving control back to the user
Use./gradlew formatKotlinto automatically fix formatting issues
Run./gradlew updateLegacyAbiafter making ABI changes to update ABI signature files
Binary compatibility is enforced - all public API changes must be tracked in the/api/directories
Validate ABI with./gradlew checkLegacyAbiand update with./gradlew updateLegacyAbi
API changes must be intentional and well-documented
Error handling follows Kotlin conventions with specific Ktor exceptions
Files:
ktor-io/common/src/io/ktor/utils/io/ByteReadChannelOperations.kt
🧠 Learnings (2)
📓 Common learnings
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:90-100
Timestamp: 2025-09-05T12:46:14.074Z
Learning: The ByteReadChannel.readAvailable(ByteBuffer) method in Ktor IO automatically calls awaitContent() internally when the read buffer is exhausted. When it returns 0, it has already suspended and waited for data to become available, so adding explicit awaitContent() calls is redundant and incorrect.
📚 Learning: 2025-09-05T12:46:14.074Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:90-100
Timestamp: 2025-09-05T12:46:14.074Z
Learning: The ByteReadChannel.readAvailable(ByteBuffer) method in Ktor IO automatically calls awaitContent() internally when the read buffer is exhausted. When it returns 0, it has already suspended and waited for data to become available, so adding explicit awaitContent() calls is redundant and incorrect.
Applied to files:
ktor-io/common/src/io/ktor/utils/io/ByteReadChannelOperations.kt
aa6049c to
a703283
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ktor-io/common/src/io/ktor/utils/io/ByteReadChannelOperations.kt (1)
185-202: Add KDoc documentation for this new public API.As per coding guidelines, all public APIs should be documented including parameters, return types, and exceptions. Consider adding documentation similar to other functions in this file.
+/** + * Reads bytes from the channel into the specified [sink] up to the given [limit]. + * + * [Report a problem](https://ktor.io/feedback/?fqname=io.ktor.utils.io.readTo) + * + * @param sink the raw sink to write the bytes to + * @param limit the maximum number of bytes to read + * @return the number of bytes read + */ @OptIn(InternalAPI::class) public suspend fun ByteReadChannel.readTo(sink: RawSink, limit: Long): Long {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/ContentTest.kt(2 hunks)ktor-io/common/src/io/ktor/utils/io/ByteReadChannelOperations.kt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.kt: Follow Kotlin official style guide (https://kotlinlang.org/docs/coding-conventions.html)
Use star imports forio.ktor.*packages
Document all public APIs including parameters, return types, and exceptions
Mark internal APIs with@InternalAPIannotation
Run./gradlew lintKotlinand fix all linting issues before giving control back to the user
Use./gradlew formatKotlinto automatically fix formatting issues
Run./gradlew updateLegacyAbiafter making ABI changes to update ABI signature files
Binary compatibility is enforced - all public API changes must be tracked in the/api/directories
Validate ABI with./gradlew checkLegacyAbiand update with./gradlew updateLegacyAbi
API changes must be intentional and well-documented
Error handling follows Kotlin conventions with specific Ktor exceptions
Files:
ktor-io/common/src/io/ktor/utils/io/ByteReadChannelOperations.ktktor-client/ktor-client-tests/common/test/io/ktor/client/tests/ContentTest.kt
**/test/**/*.kt
📄 CodeRabbit inference engine (CLAUDE.md)
Follow test naming pattern:
DescribeWhatIsBeingTested
Files:
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/ContentTest.kt
🧠 Learnings (6)
📓 Common learnings
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:90-100
Timestamp: 2025-09-05T12:46:14.074Z
Learning: The ByteReadChannel.readAvailable(ByteBuffer) method in Ktor IO automatically calls awaitContent() internally when the read buffer is exhausted. When it returns 0, it has already suspended and waited for data to become available, so adding explicit awaitContent() calls is redundant and incorrect.
📚 Learning: 2025-09-05T12:46:14.074Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:90-100
Timestamp: 2025-09-05T12:46:14.074Z
Learning: The ByteReadChannel.readAvailable(ByteBuffer) method in Ktor IO automatically calls awaitContent() internally when the read buffer is exhausted. When it returns 0, it has already suspended and waited for data to become available, so adding explicit awaitContent() calls is redundant and incorrect.
Applied to files:
ktor-io/common/src/io/ktor/utils/io/ByteReadChannelOperations.ktktor-client/ktor-client-tests/common/test/io/ktor/client/tests/ContentTest.kt
📚 Learning: 2025-11-25T09:38:19.380Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:38:19.380Z
Learning: Applies to **/*.kt : API changes must be intentional and well-documented
Applied to files:
ktor-io/common/src/io/ktor/utils/io/ByteReadChannelOperations.kt
📚 Learning: 2025-11-25T09:38:19.380Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:38:19.380Z
Learning: Applies to **/*.kt : Binary compatibility is enforced - all public API changes must be tracked in the `/api/` directories
Applied to files:
ktor-io/common/src/io/ktor/utils/io/ByteReadChannelOperations.kt
📚 Learning: 2025-05-30T06:45:52.309Z
Learnt from: rururux
Repo: ktorio/ktor PR: 4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: In Ktor test files, particularly in the ktor-client/ktor-client-core/jvm/test/ directory, test files follow the convention of not including explicit package declarations. This is consistent across test files like CachingCacheStorageTest.kt and should be maintained for consistency.
Applied to files:
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/ContentTest.kt
📚 Learning: 2025-10-22T07:21:51.263Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 5139
File: ktor-server/ktor-server-plugins/ktor-server-di/common/src/io/ktor/server/plugins/di/DependencyInjection.kt:11-11
Timestamp: 2025-10-22T07:21:51.263Z
Learning: In Ktor, `io.ktor.utils.io.CancellationException` is a typealias for `kotlinx.coroutines.CancellationException`, which is a supertype of `JobCancellationException`. Therefore, checking `e !is io.ktor.utils.io.CancellationException` is sufficient to exclude all coroutine cancellation exceptions.
Applied to files:
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/ContentTest.kt
🔇 Additional comments (3)
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/ContentTest.kt (2)
24-24: LGTM!Import added for the new
Bufferusage in the test.
140-147: Good test coverage for the newreadToutility.The test properly exercises the new
ByteReadChannel.readTo(sink, limit)function by:
- Reading response via channel instead of direct
ByteArray- Verifying the returned byte count matches expected content size
- Verifying the actual buffer content matches the original payload
ktor-io/common/src/io/ktor/utils/io/ByteReadChannelOperations.kt (1)
204-224: LGTM!The bounded
copyToimplementation correctly handles chunked reads with proper exception handling and flushing. The pattern is consistent with the newreadTofunction.
Bruce Hamilton (bjhham)
left a comment
Bruce Hamilton (bjhham)
left a comment
There was a problem hiding this comment.
This is kinda related to https://youtrack.jetbrains.com/issue/KTOR-7848/ByteChannel-Files-I-O-Improvements would be good to log it as a sub-issue and mark it as fixed for 3.4.0.
a703283 to
6dcd60c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ktor-io/common/src/io/ktor/utils/io/ByteReadChannelOperations.kt (1)
185-202: Update ABI signature files and run Kotlin linting/formatting.The new
readTo(sink: RawSink, limit: Long)function is missing from the ABI signature file (ktor-io/api/ktor-io.api). Per binary compatibility enforcement, all public API changes must be tracked in the/api/directories.Run these commands locally to complete the changes:
./gradlew updateLegacyAbi ./gradlew lintKotlin ./gradlew formatKotlin ./gradlew checkLegacyAbi
🧹 Nitpick comments (1)
ktor-io/common/src/io/ktor/utils/io/ByteReadChannelOperations.kt (1)
185-202: Add finally block to ensure sink is flushed.The
copyTofunction at lines 219-221 uses a finally block to ensure the channel is flushed even if an exception occurs. For consistency and robustness,readToshould similarly ensure the sink is flushed in a finally block.Apply this diff:
@OptIn(InternalAPI::class) public suspend fun ByteReadChannel.readTo(sink: RawSink, limit: Long = Long.MAX_VALUE): Long { var remaining = limit try { while (!isClosedForRead && remaining > 0) { if (readBuffer.exhausted()) awaitContent() val byteCount = minOf(remaining, readBuffer.remaining) readBuffer.readTo(sink, byteCount) remaining -= byteCount sink.flush() } } catch (cause: Throwable) { cancel(cause) throw cause + } finally { + sink.flush() } return limit - remaining }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/ContentTest.kt(2 hunks)ktor-io/common/src/io/ktor/utils/io/ByteReadChannelOperations.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/ContentTest.kt
🧰 Additional context used
📓 Path-based instructions (1)
**/*.kt
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.kt: Follow Kotlin official style guide (https://kotlinlang.org/docs/coding-conventions.html)
Use star imports forio.ktor.*packages
Document all public APIs including parameters, return types, and exceptions
Mark internal APIs with@InternalAPIannotation
Run./gradlew lintKotlinand fix all linting issues before giving control back to the user
Use./gradlew formatKotlinto automatically fix formatting issues
Run./gradlew updateLegacyAbiafter making ABI changes to update ABI signature files
Binary compatibility is enforced - all public API changes must be tracked in the/api/directories
Validate ABI with./gradlew checkLegacyAbiand update with./gradlew updateLegacyAbi
API changes must be intentional and well-documented
Error handling follows Kotlin conventions with specific Ktor exceptions
Files:
ktor-io/common/src/io/ktor/utils/io/ByteReadChannelOperations.kt
🧠 Learnings (3)
📓 Common learnings
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:90-100
Timestamp: 2025-09-05T12:46:14.074Z
Learning: The ByteReadChannel.readAvailable(ByteBuffer) method in Ktor IO automatically calls awaitContent() internally when the read buffer is exhausted. When it returns 0, it has already suspended and waited for data to become available, so adding explicit awaitContent() calls is redundant and incorrect.
📚 Learning: 2025-09-05T12:46:14.074Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:90-100
Timestamp: 2025-09-05T12:46:14.074Z
Learning: The ByteReadChannel.readAvailable(ByteBuffer) method in Ktor IO automatically calls awaitContent() internally when the read buffer is exhausted. When it returns 0, it has already suspended and waited for data to become available, so adding explicit awaitContent() calls is redundant and incorrect.
Applied to files:
ktor-io/common/src/io/ktor/utils/io/ByteReadChannelOperations.kt
📚 Learning: 2025-11-25T09:38:19.380Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:38:19.380Z
Learning: Applies to **/*.kt : API changes must be intentional and well-documented
Applied to files:
ktor-io/common/src/io/ktor/utils/io/ByteReadChannelOperations.kt
🧬 Code graph analysis (1)
ktor-io/common/src/io/ktor/utils/io/ByteReadChannelOperations.kt (3)
ktor-io/common/src/io/ktor/utils/io/ByteChannel.kt (2)
awaitContent(69-80)cancel(136-144)ktor-io/common/src/io/ktor/utils/io/CountedByteReadChannel.kt (2)
awaitContent(43-53)cancel(62-65)ktor-io/jvm/src/io/ktor/utils/io/jvm/javaio/Reading.kt (2)
awaitContent(66-87)cancel(89-94)
Leonid Stashevsky (e5l)
left a comment
Leonid Stashevsky (e5l)
left a comment
There was a problem hiding this comment.
Hey Pantus Oleh (@zibet27), nice PR. Please check the comment before merging
| ) | ||
| ) | ||
| }.body<ByteArray>() | ||
| }.bodyAsChannel() |
There was a problem hiding this comment.
Should we have tests for 2 separate receive types?
| public suspend fun ByteReadChannel.readTo(sink: RawSink, limit: Long = Long.MAX_VALUE): Long { | ||
| var remaining = limit | ||
| try { | ||
| while (!isClosedForRead && remaining > 0) { |
There was a problem hiding this comment.
I would also have a test with cancellation
| } | ||
|
|
||
| @OptIn(InternalAPI::class) | ||
| public suspend fun ByteReadChannel.readTo(sink: RawSink, limit: Long = Long.MAX_VALUE): Long { |
There was a problem hiding this comment.
Please add KDoc
6dcd60c to
e0ec75b
Compare
e0ec75b to
f1b9bfe
Compare
3 participants
Subsystem
Shared
Motivation
Github Issue
Solution
Reuse some existing code, but make the interface more general
Update:
Unifying
ByteReadChannel.readTo(sink: RawSink, limit: Long)withByteReadChannel.copyTo(channel: ByteWriteChannel, limit: Long)breaks some internal logic. Although we could try making ByteReadChannel implement the RawSink interface, it is still somewhat different.So, returning to code duplication.