KTOR-9606 Fix KotlinxSerializationConverter deserialization of empty channel#5652
Open
Aleksei Tirman (Stexxe) wants to merge 5 commits into
Open
KTOR-9606 Fix KotlinxSerializationConverter deserialization of empty channel#5652Aleksei Tirman (Stexxe) wants to merge 5 commits into
Aleksei Tirman (Stexxe) wants to merge 5 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors Kotlinx deserialization to buffer remaining input and probe extensions from that buffer; adds JVM tests for Kotlinx, Gson, Jackson, and Jackson3 converters that exercise deserialization from an initially empty ByteChannel closed after a short delay. ChangesEmpty Channel Handling in Serialization Converters
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
ktor-shared/ktor-serialization/ktor-serialization-jackson3/jvm/test/ConverterTest.kt (1)
25-25: ⚡ Quick winUse a backticked descriptive test name.
Please rename this test to backtick style (for example,
fun `throws exception for empty channel`()) to align with test naming rules.As per coding guidelines
**/*Test.{kt,kts}: Prefer descriptive test names in backticks:describe what is being tested.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ktor-shared/ktor-serialization/ktor-serialization-jackson3/jvm/test/ConverterTest.kt` at line 25, Rename the test function throwsExceptionForEmptyChannel to a backticked descriptive name (e.g. `fun \`throws exception for empty channel\`()`) to follow the project’s test naming convention; locate the function definition in ConverterTest (the one currently declared as throwsExceptionForEmptyChannel = runTest { ... }) and change its identifier to a backticked, human-readable phrase while keeping the existing body and runTest call unchanged.ktor-shared/ktor-serialization/ktor-serialization-gson/jvm/test/ConverterTest.kt (1)
22-22: ⚡ Quick winUse a backticked descriptive test name.
Please rename this test to backtick style (for example,
fun `returns null for empty channel with delayed close`()) to align with test naming rules.As per coding guidelines
**/*Test.{kt,kts}: Prefer descriptive test names in backticks:describe what is being tested.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ktor-shared/ktor-serialization/ktor-serialization-gson/jvm/test/ConverterTest.kt` at line 22, Rename the test function returnsNullForEmptyChannelWithDelayedClose to use a backticked descriptive name (e.g. `returns null for empty channel with delayed close`) in ConverterTest.kt so it follows the project's test naming convention; update the function declaration for returnsNullForEmptyChannelWithDelayedClose to the backticked form and ensure any references (invocations or annotations) remain valid.ktor-shared/ktor-serialization/ktor-serialization-kotlinx/common/test/io/ktor/serialization/kotlinx/ConverterTest.kt (1)
24-24: ⚡ Quick winUse a backticked descriptive test name.
Please rename this test to backtick style (for example,
fun `returns null for empty channel with delayed close`()) to match the repository’s test naming convention.As per coding guidelines
**/*Test.{kt,kts}: Prefer descriptive test names in backticks:describe what is being tested.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ktor-shared/ktor-serialization/ktor-serialization-kotlinx/common/test/io/ktor/serialization/kotlinx/ConverterTest.kt` at line 24, Rename the test function returnsNullForEmptyChannelWithDelayedClose to a backticked descriptive name following the repo convention, e.g. change the function declaration to use a backticked identifier like `fun `returns null for empty channel with delayed close`()`, updating any references to the function name (if any) so the test runner executes it correctly; modify the declaration in ConverterTest.kt where returnsNullForEmptyChannelWithDelayedClose is defined.ktor-shared/ktor-serialization/ktor-serialization-jackson/jvm/test/ConverterTest.kt (1)
25-25: ⚡ Quick winUse a backticked descriptive test name.
Please rename this test to backtick style (for example,
fun `throws exception for empty channel`()) to align with test naming rules.As per coding guidelines
**/*Test.{kt,kts}: Prefer descriptive test names in backticks:describe what is being tested.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ktor-shared/ktor-serialization/ktor-serialization-jackson/jvm/test/ConverterTest.kt` at line 25, Rename the test function throwsExceptionForEmptyChannel to a descriptive backticked name (e.g. fun `throws exception for empty channel`() = runTest { ... }) to follow the test-naming guideline; update the function declaration in ConverterTest (keep the existing runTest block and body unchanged) and adjust any internal references or imports if the symbol is referenced elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@ktor-shared/ktor-serialization/ktor-serialization-gson/jvm/test/ConverterTest.kt`:
- Line 22: Rename the test function returnsNullForEmptyChannelWithDelayedClose
to use a backticked descriptive name (e.g. `returns null for empty channel with
delayed close`) in ConverterTest.kt so it follows the project's test naming
convention; update the function declaration for
returnsNullForEmptyChannelWithDelayedClose to the backticked form and ensure any
references (invocations or annotations) remain valid.
In
`@ktor-shared/ktor-serialization/ktor-serialization-jackson/jvm/test/ConverterTest.kt`:
- Line 25: Rename the test function throwsExceptionForEmptyChannel to a
descriptive backticked name (e.g. fun `throws exception for empty channel`() =
runTest { ... }) to follow the test-naming guideline; update the function
declaration in ConverterTest (keep the existing runTest block and body
unchanged) and adjust any internal references or imports if the symbol is
referenced elsewhere.
In
`@ktor-shared/ktor-serialization/ktor-serialization-jackson3/jvm/test/ConverterTest.kt`:
- Line 25: Rename the test function throwsExceptionForEmptyChannel to a
backticked descriptive name (e.g. `fun \`throws exception for empty
channel\`()`) to follow the project’s test naming convention; locate the
function definition in ConverterTest (the one currently declared as
throwsExceptionForEmptyChannel = runTest { ... }) and change its identifier to a
backticked, human-readable phrase while keeping the existing body and runTest
call unchanged.
In
`@ktor-shared/ktor-serialization/ktor-serialization-kotlinx/common/test/io/ktor/serialization/kotlinx/ConverterTest.kt`:
- Line 24: Rename the test function returnsNullForEmptyChannelWithDelayedClose
to a backticked descriptive name following the repo convention, e.g. change the
function declaration to use a backticked identifier like `fun `returns null for
empty channel with delayed close`()`, updating any references to the function
name (if any) so the test runner executes it correctly; modify the declaration
in ConverterTest.kt where returnsNullForEmptyChannelWithDelayedClose is defined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e0eba00-f63d-465e-bf7a-657fb16371a7
📒 Files selected for processing (6)
ktor-shared/ktor-serialization/ktor-serialization-gson/jvm/test/ConverterTest.ktktor-shared/ktor-serialization/ktor-serialization-jackson/jvm/test/ConverterTest.ktktor-shared/ktor-serialization/ktor-serialization-jackson3/jvm/test/ConverterTest.ktktor-shared/ktor-serialization/ktor-serialization-kotlinx/build.gradle.ktsktor-shared/ktor-serialization/ktor-serialization-kotlinx/common/src/io/ktor/serialization/kotlinx/KotlinxSerializationConverter.ktktor-shared/ktor-serialization/ktor-serialization-kotlinx/common/test/io/ktor/serialization/kotlinx/ConverterTest.kt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ktor-shared/ktor-serialization/ktor-serialization-kotlinx/common/src/io/ktor/serialization/kotlinx/KotlinxSerializationConverter.kt`:
- Around line 60-63: The empty-body fast path is currently gated by
extensions.isNotEmpty(), making it format-dependent; update the logic in
KotlinxSerializationConverter (the block computing fromExtension using
extensions.asFlow().map { it.deserialize(charset, typeInfo,
ByteReadChannel(contentPacket)) }.firstOrNull { ... }) so that
contentPacket.exhausted() is checked independently of extensions.isNotEmpty():
first attempt to short-circuit and return when contentPacket.exhausted()
(format-agnostic), otherwise evaluate fromExtension and only then return it when
extensions.isNotEmpty() and fromExtension != null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67045ed0-ffcc-4c85-a1b5-76d04492abba
📒 Files selected for processing (1)
ktor-shared/ktor-serialization/ktor-serialization-kotlinx/common/src/io/ktor/serialization/kotlinx/KotlinxSerializationConverter.kt
Bruce Hamilton (bjhham)
requested changes
May 27, 2026
Aleksei Tirman (Stexxe)
force-pushed
the
stexxe/converter-delayed-close
branch
from
bdc6d0d to
2bdb9b5
Compare
Osip Fatkullin (osipxd)
requested changes
May 27, 2026
Osip Fatkullin (osipxd)
left a comment
Osip Fatkullin (osipxd)
left a comment
Member
There was a problem hiding this comment.
Offtop:
It feels weird that we have three ways receive payload:
call.receive<Payload>()call.receiveNullable<Payload>()call.receiveNullable<Payload?>()
But this is out of the scope of this PR.
Osip Fatkullin (osipxd)
approved these changes
May 28, 2026
Member
|
It seems that my comment was actually wrong. This commit eb14d53 has added |
…ed with a delay empty channel
This reverts commit 85ddd9e.
Aleksei Tirman (Stexxe)
force-pushed
the
stexxe/converter-delayed-close
branch
from
85ddd9e to
7240236
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
ktor-shared/ktor-serialization/ktor-serialization-kotlinx/common/test/io/ktor/serialization/kotlinx/ConverterTest.kt (2)
24-24: ⚡ Quick winPrefer backtick test names for readability and consistency.
Rename this test to a descriptive backtick form.
Suggested diff
- fun returnsNullForEmptyChannelWithDelayedClose() = runTest { + fun `returns null for empty channel with delayed close`() = runTest {As per coding guidelines: "Prefer descriptive test names in backticks:
describe what is being tested".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ktor-shared/ktor-serialization/ktor-serialization-kotlinx/common/test/io/ktor/serialization/kotlinx/ConverterTest.kt` at line 24, The test function returnsNullForEmptyChannelWithDelayedClose should be renamed to a descriptive backtick-style name for readability and consistency; locate the function returnsNullForEmptyChannelWithDelayedClose in ConverterTest.kt and change its declaration to use a backtick name like `returns null for empty channel with delayed close` (i.e., fun `returns null for empty channel with delayed close`() = runTest { ... }) ensuring any internal references or test frameworks pick up the new name.
7-10: ⚡ Quick winUse
io.ktor.*star imports to match repository import policy.These imports should follow the configured star-import rule for Ktor packages.
Suggested diff
-import io.ktor.serialization.kotlinx.json.DefaultJson -import io.ktor.util.reflect.typeInfo -import io.ktor.utils.io.ByteChannel -import io.ktor.utils.io.charsets.Charsets +import io.ktor.serialization.kotlinx.json.* +import io.ktor.util.reflect.* +import io.ktor.utils.io.* +import io.ktor.utils.io.charsets.*As per coding guidelines: "Use star imports for
io.ktor.*(as configured in .editorconfig)".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ktor-shared/ktor-serialization/ktor-serialization-kotlinx/common/test/io/ktor/serialization/kotlinx/ConverterTest.kt` around lines 7 - 10, Replace the explicit Ktor imports in ConverterTest.kt (e.g., io.ktor.serialization.kotlinx.json.DefaultJson, io.ktor.util.reflect.typeInfo, io.ktor.utils.io.ByteChannel, io.ktor.utils.io.charsets.Charsets) with the repository's configured star-style import for Ktor packages (use a single import like io.ktor.* or the appropriate io.ktor.* grouping) so the file follows the .editorconfig rule; ensure no other non-Ktor symbols are collapsed and run the project linter to confirm the import style passes.ktor-shared/ktor-serialization/ktor-serialization-gson/jvm/test/ConverterTest.kt (2)
22-22: ⚡ Quick winUse a descriptive backtick test name.
Please rename this test to the preferred backtick style.
Suggested diff
- fun returnsNullForEmptyChannelWithDelayedClose() = runTest { + fun `returns null for empty channel with delayed close`() = runTest {As per coding guidelines: "Prefer descriptive test names in backticks:
describe what is being tested".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ktor-shared/ktor-serialization/ktor-serialization-gson/jvm/test/ConverterTest.kt` at line 22, Rename the test function returnsNullForEmptyChannelWithDelayedClose to a descriptive backtick-style name by changing its declaration to use a backticked string (e.g. fun `returns null for empty channel with delayed close`() = runTest { ... }) so the test reads like a sentence; update the function name in ConverterTest.kt wherever referenced and keep the existing body and runTest usage unchanged.
5-8: ⚡ Quick winSwitch
io.ktor.*imports to star imports per project rules.The current explicit Ktor imports conflict with repository import configuration.
Suggested diff
-import io.ktor.serialization.gson.GsonConverter -import io.ktor.util.reflect.typeInfo -import io.ktor.utils.io.ByteChannel -import io.ktor.utils.io.charsets.Charsets +import io.ktor.serialization.gson.* +import io.ktor.util.reflect.* +import io.ktor.utils.io.* +import io.ktor.utils.io.charsets.*As per coding guidelines: "Use star imports for
io.ktor.*(as configured in .editorconfig)".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ktor-shared/ktor-serialization/ktor-serialization-gson/jvm/test/ConverterTest.kt` around lines 5 - 8, The imports from io.ktor.* (GsonConverter, typeInfo, ByteChannel, Charsets) should use star imports to match the repo's .editorconfig rule; replace the explicit io.ktor.serialization.gson.GsonConverter, io.ktor.util.reflect.typeInfo, io.ktor.utils.io.ByteChannel and io.ktor.utils.io.charsets.Charsets imports with the appropriate io.ktor.* wildcard imports so the file follows the project's import convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@ktor-shared/ktor-serialization/ktor-serialization-gson/jvm/test/ConverterTest.kt`:
- Line 22: Rename the test function returnsNullForEmptyChannelWithDelayedClose
to a descriptive backtick-style name by changing its declaration to use a
backticked string (e.g. fun `returns null for empty channel with delayed
close`() = runTest { ... }) so the test reads like a sentence; update the
function name in ConverterTest.kt wherever referenced and keep the existing body
and runTest usage unchanged.
- Around line 5-8: The imports from io.ktor.* (GsonConverter, typeInfo,
ByteChannel, Charsets) should use star imports to match the repo's .editorconfig
rule; replace the explicit io.ktor.serialization.gson.GsonConverter,
io.ktor.util.reflect.typeInfo, io.ktor.utils.io.ByteChannel and
io.ktor.utils.io.charsets.Charsets imports with the appropriate io.ktor.*
wildcard imports so the file follows the project's import convention.
In
`@ktor-shared/ktor-serialization/ktor-serialization-kotlinx/common/test/io/ktor/serialization/kotlinx/ConverterTest.kt`:
- Line 24: The test function returnsNullForEmptyChannelWithDelayedClose should
be renamed to a descriptive backtick-style name for readability and consistency;
locate the function returnsNullForEmptyChannelWithDelayedClose in
ConverterTest.kt and change its declaration to use a backtick name like `returns
null for empty channel with delayed close` (i.e., fun `returns null for empty
channel with delayed close`() = runTest { ... }) ensuring any internal
references or test frameworks pick up the new name.
- Around line 7-10: Replace the explicit Ktor imports in ConverterTest.kt (e.g.,
io.ktor.serialization.kotlinx.json.DefaultJson, io.ktor.util.reflect.typeInfo,
io.ktor.utils.io.ByteChannel, io.ktor.utils.io.charsets.Charsets) with the
repository's configured star-style import for Ktor packages (use a single import
like io.ktor.* or the appropriate io.ktor.* grouping) so the file follows the
.editorconfig rule; ensure no other non-Ktor symbols are collapsed and run the
project linter to confirm the import style passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dd7de9b6-b0db-4208-a822-052bd67897aa
📒 Files selected for processing (6)
ktor-shared/ktor-serialization/ktor-serialization-gson/jvm/test/ConverterTest.ktktor-shared/ktor-serialization/ktor-serialization-jackson/jvm/test/ConverterTest.ktktor-shared/ktor-serialization/ktor-serialization-jackson3/jvm/test/ConverterTest.ktktor-shared/ktor-serialization/ktor-serialization-kotlinx/build.gradle.ktsktor-shared/ktor-serialization/ktor-serialization-kotlinx/common/src/io/ktor/serialization/kotlinx/KotlinxSerializationConverter.ktktor-shared/ktor-serialization/ktor-serialization-kotlinx/common/test/io/ktor/serialization/kotlinx/ConverterTest.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- ktor-shared/ktor-serialization/ktor-serialization-kotlinx/build.gradle.kts
- ktor-shared/ktor-serialization/ktor-serialization-jackson3/jvm/test/ConverterTest.kt
- ktor-shared/ktor-serialization/ktor-serialization-jackson/jvm/test/ConverterTest.kt
- ktor-shared/ktor-serialization/ktor-serialization-kotlinx/common/src/io/ktor/serialization/kotlinx/KotlinxSerializationConverter.kt
Aleksei Tirman (Stexxe)
requested review from
Bruce Hamilton (bjhham)
and removed request for
Bruce Hamilton (bjhham)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://youtrack.jetbrains.com/issue/KTOR-9606