Skip to content

Conversation

@marychatte
Copy link
Member

Subsystem
Server, SSE

Motivation
KTOR-9108 SSE: The handler adds Connection: Keep-Alive header, which is incompatible with HTTP/2

@marychatte marychatte requested a review from osipxd November 26, 2025 16:27
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 26, 2025

Walkthrough

Emit Connection: keep-alive for SSE responses only when the incoming request uses HTTP/1.0 or HTTP/1.1; add SSE dependency to test-suite and add a test that verifies Connection header is absent on HTTP/2.

Changes

Cohort / File(s) Change Summary
SSE Connection Header Logic
ktor-server/ktor-server-plugins/ktor-server-sse/common/src/io/ktor/server/sse/Routing.kt
Make emission of Connection: keep-alive conditional on request HTTP version (HTTP/1.0 or HTTP/1.1). Added import io.ktor.server.request.*.
Test-suite Dependency
ktor-server/ktor-server-test-suites/build.gradle.kts
Add implementation(projects.ktorServerSse) to commonMain dependencies.
SSE HTTP/2 Test
ktor-server/ktor-server-test-suites/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.kt
Add import io.ktor.server.sse.* and new test testSSEDoesNotSendConnectionOnHttp2 that installs SSE, emits an event, requests text/event-stream, and asserts no Connection header on HTTP/2 plus content checks.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~15-25 minutes

  • Verify the HTTP version check logic and edge cases (e.g., proxies, version normalization).
  • Confirm SSE behavior on HTTP/2/HTTP/3 and that no unintended headers are emitted.
  • Review the added test for reliability across server backends and correct test wiring after dependency addition.

Suggested reviewers

  • osipxd
  • bjhham

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check ❓ Inconclusive The description includes Subsystem and Motivation sections as required by the template. However, the Solution section is missing, which should describe how the keep-alive header emission was made conditional for HTTP/1.x. Add a Solution section describing the implementation approach, such as making the keep-alive header conditional based on HTTP version.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: removing the Connection: keep-alive header from SSE for HTTP/2 requests, which directly corresponds to the code modification in Routing.kt.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch marychatte/marychatte/KTOR-9108-SSE-Connection-Keep-Alive-header

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
ktor-server/ktor-server-plugins/ktor-server-sse/common/src/io/ktor/server/sse/Routing.kt (1)

179-188: Connection: keep-alive now correctly limited to HTTP/1.x only

The new condition ensures Connection: keep-alive is sent only for HTTP/1.1 and HTTP/1.0, avoiding the header for HTTP/2+ where it is not allowed, while preserving existing behavior for HTTP/1.x SSE. The logic is clear and localized. If not already covered, consider adding/adjusting tests to assert the header is absent for HTTP/2 and present for HTTP/1.x.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 315f8f2 and f261520.

📒 Files selected for processing (1)
  • ktor-server/ktor-server-plugins/ktor-server-sse/common/src/io/ktor/server/sse/Routing.kt (2 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 for io.ktor.* packages
Document all public APIs including parameters, return types, and exceptions
Mark internal APIs with @InternalAPI annotation
Run ./gradlew lintKotlin and fix all linting issues before giving control back to the user
Use ./gradlew formatKotlin to automatically fix formatting issues
Run ./gradlew updateLegacyAbi after 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 checkLegacyAbi and update with ./gradlew updateLegacyAbi
API changes must be intentional and well-documented
Error handling follows Kotlin conventions with specific Ktor exceptions

Files:

  • ktor-server/ktor-server-plugins/ktor-server-sse/common/src/io/ktor/server/sse/Routing.kt
🧠 Learnings (5)
📚 Learning: 2025-05-16T13:11:28.416Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 4860
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.api:24-26
Timestamp: 2025-05-16T13:11:28.416Z
Learning: Binary incompatible changes (such as constructor signature changes) are acceptable in the ktor-server-di module when the version is not yet released, as confirmed by the development team.

Applied to files:

  • ktor-server/ktor-server-plugins/ktor-server-sse/common/src/io/ktor/server/sse/Routing.kt
📚 Learning: 2025-11-25T09:38:19.393Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:38:19.393Z
Learning: Applies to **/*.kt : Use star imports for `io.ktor.*` packages

Applied to files:

  • ktor-server/ktor-server-plugins/ktor-server-sse/common/src/io/ktor/server/sse/Routing.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: The headersOf() function from io.ktor.http package is available through wildcard imports like `import io.ktor.http.*`, so no explicit import statement is needed when using wildcard imports from that package.

Applied to files:

  • ktor-server/ktor-server-plugins/ktor-server-sse/common/src/io/ktor/server/sse/Routing.kt
📚 Learning: 2025-11-25T09:38:19.393Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:38:19.393Z
Learning: Applies to **/*.kt : Error handling follows Kotlin conventions with specific Ktor exceptions

Applied to files:

  • ktor-server/ktor-server-plugins/ktor-server-sse/common/src/io/ktor/server/sse/Routing.kt
📚 Learning: 2025-06-23T12:49:56.883Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: Error handling should follow Kotlin conventions and use specific Ktor exceptions.

Applied to files:

  • ktor-server/ktor-server-plugins/ktor-server-sse/common/src/io/ktor/server/sse/Routing.kt
🔇 Additional comments (1)
ktor-server/ktor-server-plugins/ktor-server-sse/common/src/io/ktor/server/sse/Routing.kt (1)

7-9: Import of io.ktor.server.request.* is appropriate and consistent

The added star import is needed for call.request.httpVersion below and matches the guideline to use star imports for io.ktor.* packages. No issues here.

As per coding guidelines, star imports for io.ktor.* are preferred.

Copy link
Member

@osipxd osipxd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
Is it possible to test this?

@marychatte marychatte force-pushed the marychatte/marychatte/KTOR-9108-SSE-Connection-Keep-Alive-header branch 2 times, most recently from 1eb5f1a to e342081 Compare November 26, 2025 18:46
@marychatte
Copy link
Member Author

@osipxd I added a test to check the header Connection

@marychatte marychatte force-pushed the marychatte/marychatte/KTOR-9108-SSE-Connection-Keep-Alive-header branch from e342081 to 314646f Compare November 26, 2025 20:00
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1eb5f1a and 314646f.

📒 Files selected for processing (3)
  • ktor-server/ktor-server-plugins/ktor-server-sse/common/src/io/ktor/server/sse/Routing.kt (2 hunks)
  • ktor-server/ktor-server-test-suites/build.gradle.kts (1 hunks)
  • ktor-server/ktor-server-test-suites/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.kt (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • ktor-server/ktor-server-plugins/ktor-server-sse/common/src/io/ktor/server/sse/Routing.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 for io.ktor.* packages
Document all public APIs including parameters, return types, and exceptions
Mark internal APIs with @InternalAPI annotation
Run ./gradlew lintKotlin and fix all linting issues before giving control back to the user
Use ./gradlew formatKotlin to automatically fix formatting issues
Run ./gradlew updateLegacyAbi after 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 checkLegacyAbi and update with ./gradlew updateLegacyAbi
API changes must be intentional and well-documented
Error handling follows Kotlin conventions with specific Ktor exceptions

Files:

  • ktor-server/ktor-server-test-suites/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.kt
🧠 Learnings (12)
📚 Learning: 2025-08-14T15:17:11.466Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts:12-12
Timestamp: 2025-08-14T15:17:11.466Z
Learning: The Cargo plugin (dev.gobley.cargo) used in the Ktor WebRTC RS module depends on the kotlin("plugin.atomicfu") plugin, so atomicfu should not be removed even if there are no direct kotlinx.atomicfu imports in the module's source code.

Applied to files:

  • ktor-server/ktor-server-test-suites/build.gradle.kts
📚 Learning: 2025-08-14T15:17:11.466Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts:12-12
Timestamp: 2025-08-14T15:17:11.466Z
Learning: The Gobley Cargo plugin (dev.gobley.cargo) used in the Ktor WebRTC RS module requires the kotlin("plugin.atomicfu") plugin as a dependency, so the atomicfu plugin should not be removed even if there are no direct kotlinx.atomicfu imports in the module's source code.

Applied to files:

  • ktor-server/ktor-server-test-suites/build.gradle.kts
📚 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-server/ktor-server-test-suites/build.gradle.kts
📚 Learning: 2025-05-08T09:52:11.723Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 4836
File: build-logic/src/main/kotlin/ktorbuild.project.server-plugin.gradle.kts:12-12
Timestamp: 2025-05-08T09:52:11.723Z
Learning: In the Ktor project's flattened Gradle structure, projects are declared using a custom DSL with the unary plus operator inside specialized blocks. For example, server projects are declared with `+"project-name"` inside a `server {}` block within the main `projects {}` block. When verifying project declarations, search for `+"project-name"` rather than traditional `include(":project-name")` statements.

Applied to files:

  • ktor-server/ktor-server-test-suites/build.gradle.kts
📚 Learning: 2025-11-25T09:38:19.393Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:38:19.393Z
Learning: Applies to **/*.kt : Use star imports for `io.ktor.*` packages

Applied to files:

  • ktor-server/ktor-server-test-suites/build.gradle.kts
  • ktor-server/ktor-server-test-suites/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.kt
📚 Learning: 2025-05-07T09:12:14.293Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 4836
File: ktor-utils/build.gradle.kts:35-35
Timestamp: 2025-05-07T09:12:14.293Z
Learning: The Ktor project maintains a flat Gradle project structure (where projects are referenced without nested paths like `:ktor-test-base`) while keeping a hierarchical directory organization on disk.

Applied to files:

  • ktor-server/ktor-server-test-suites/build.gradle.kts
📚 Learning: 2025-11-14T14:11:30.292Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 5195
File: ktor-client/ktor-client-java/jvm/test/io/ktor/client/engine/java/JavaHttp2Test.kt:13-15
Timestamp: 2025-11-14T14:11:30.292Z
Learning: In ktor-client-java tests, Java's HttpClient does support h2c (HTTP/2 cleartext) when configured with `protocolVersion = HttpClient.Version.HTTP_2` on Java 11+. The JavaHttp2Test successfully passes with the default useH2c=true setting, connecting to http://localhost:8084.

Applied to files:

  • ktor-server/ktor-server-test-suites/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.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: The headersOf() function from io.ktor.http package is available through wildcard imports like `import io.ktor.http.*`, so no explicit import statement is needed when using wildcard imports from that package.

Applied to files:

  • ktor-server/ktor-server-test-suites/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.kt
📚 Learning: 2025-11-25T09:38:19.393Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:38:19.393Z
Learning: Applies to **/*.kt : Error handling follows Kotlin conventions with specific Ktor exceptions

Applied to files:

  • ktor-server/ktor-server-test-suites/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.kt
📚 Learning: 2025-06-23T12:49:56.883Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: Error handling should follow Kotlin conventions and use specific Ktor exceptions.

Applied to files:

  • ktor-server/ktor-server-test-suites/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.kt
📚 Learning: 2025-09-05T12:47:49.016Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:35-52
Timestamp: 2025-09-05T12:47:49.016Z
Learning: JettyWebsocketConnection in ktor-server-jetty-jakarta implements Closeable interface (has close() method), so connection.use { } is valid Kotlin syntax and will compile correctly.

Applied to files:

  • ktor-server/ktor-server-test-suites/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.kt
📚 Learning: 2025-06-16T17:03:45.729Z
Learnt from: tre3p
Repo: ktorio/ktor PR: 4936
File: ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt:28-35
Timestamp: 2025-06-16T17:03:45.729Z
Learning: In the Ktor codebase, encoder implementations consistently use GlobalScope.writer and GlobalScope.reader patterns for compression/decompression operations. This is the established architectural pattern used throughout the codebase for byte channel transformations.

Applied to files:

  • ktor-server/ktor-server-test-suites/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.kt
🔇 Additional comments (2)
ktor-server/ktor-server-test-suites/build.gradle.kts (1)

19-19: LGTM!

The SSE dependency addition is appropriate and enables SSE testing in the common test suite, aligning with the PR objective to test SSE behavior with HTTP/2.

ktor-server/ktor-server-test-suites/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.kt (1)

25-25: LGTM!

The star import for SSE follows the Ktor coding guidelines and is necessary for the new test function.

@marychatte marychatte enabled auto-merge (squash) November 27, 2025 14:29
@marychatte marychatte disabled auto-merge December 3, 2025 11:33
@marychatte marychatte enabled auto-merge (squash) December 3, 2025 11:33
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.

3 participants