KTOR-6963 Fix for Darwin engine ws frame limit #5013
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new internal attribute key 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)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/websocket/WebSockets.kt(2 hunks)ktor-client/ktor-client-darwin/darwin/src/io/ktor/client/engine/darwin/internal/DarwinSession.kt(2 hunks)ktor-client/ktor-client-darwin/darwin/test/DarwinEngineTest.kt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{kt,kts}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
**/*.{kt,kts,json,yaml,yml}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
**/test/**/*.kt
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
PR: ktorio/ktor#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:41:41.935Z
Learning: Prefix commit messages with KTOR-{NUM} (YouTrack issue)
Learnt from: osipxd
PR: ktorio/ktor#4970
File: build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts:67-72
Timestamp: 2025-07-01T10:54:53.751Z
Learning: In build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts, the KT-70915 and KT-77609 workarounds that limit parallelism for KotlinNativeLink and CInteropCommonizerTask should remain unconditional (not gated behind CI flag) as these limits are beneficial for both local and CI builds.
Learnt from: osipxd
PR: ktorio/ktor#4856
File: teamcity.default.properties:5-5
Timestamp: 2025-05-14T15:56:33.598Z
Learning: The Ktor project runs binary-compatibility-validator tasks on CI which require substantial metaspace, so adding metaspace size limits (-XX:MaxMetaspaceSize) to Gradle options would cause these tasks to fail.
Learnt from: CR
PR: ktorio/ktor#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: Prefix commit messages with KTOR-{NUM} (YouTrack issue), and write them in present tense, imperative mood.
Learnt from: bjhham
PR: ktorio/ktor#4916
File: ktor-server/ktor-server-core/api/ktor-server-core.api:151-151
Timestamp: 2025-06-09T07:08:35.085Z
Learning: Breaking changes are acceptable in Ktor codebase when the code hasn't been released yet, as confirmed by bjhham from the development team.
ktor-client/ktor-client-darwin/darwin/src/io/ktor/client/engine/darwin/internal/DarwinSession.kt (7)
Learnt from: osipxd
PR: ktorio/ktor#4856
File: teamcity.default.properties:5-5
Timestamp: 2025-05-14T15:56:33.598Z
Learning: The Ktor project runs binary-compatibility-validator tasks on CI which require substantial metaspace, so adding metaspace size limits (-XX:MaxMetaspaceSize) to Gradle options would cause these tasks to fail.
Learnt from: rururux
PR: ktorio/ktor#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.
Learnt from: rururux
PR: ktorio/ktor#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.
Learnt from: tre3p
PR: ktorio/ktor#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.
Learnt from: CR
PR: ktorio/ktor#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:41:41.935Z
Learning: Applies to **/*.{kt,kts} : Error handling follows Kotlin conventions with specific Ktor exceptions
Learnt from: CR
PR: ktorio/ktor#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.
Learnt from: CR
PR: ktorio/ktor#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:41:41.935Z
Learning: Applies to **/*.{kt,kts} : Use star imports for io.ktor.* packages (configured in .editorconfig)
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/websocket/WebSockets.kt (1)
Learnt from: bjhham
PR: ktorio/ktor#4855
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.klib.api:334-336
Timestamp: 2025-05-14T18:05:02.321Z
Learning: Breaking changes in constructor parameter order are acceptable for the ktor-server-di module when the code hasn't been released yet, as confirmed by the development team.
ktor-client/ktor-client-darwin/darwin/test/DarwinEngineTest.kt (1)
Learnt from: CR
PR: ktorio/ktor#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.
🧬 Code Graph Analysis (1)
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/websocket/WebSockets.kt (1)
ktor-utils/common/src/io/ktor/util/Attributes.kt (1)
AttributeKey(19-21)
🔇 Additional comments (3)
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/websocket/WebSockets.kt (2)
22-23: LGTM: Well-implemented internal attribute keyThe new
WEBSOCKETS_KEYfollows the established pattern ofREQUEST_EXTENSIONS_KEYand is correctly annotated with@InternalAPIsince it's intended for internal engine usage. The attribute name clearly indicates its purpose.
190-190: LGTM: Proper placement of plugin instance storageThe plugin instance is correctly stored in the request attributes after extension installation and before proceeding with WebSocket content. This enables Darwin engine to access the plugin configuration during WebSocket setup.
ktor-client/ktor-client-darwin/darwin/src/io/ktor/client/engine/darwin/internal/DarwinSession.kt (1)
36-38: Document magic ratio for Darwin WebSocket frame sizeIt looks like
WEBSOCKETS_KEYis always set by the WebSockets plugin for upgrade requests, so a null-check aroundrequest.attributes[WEBSOCKETS_KEY]isn’t needed. However, dividingmaxFrameSizeby 10 is a “magic number” that needs clarifying.• File: ktor-client/ktor-client-darwin/darwin/src/io/ktor/client/engine/darwin/internal/DarwinSession.kt
Lines: 36–38Please add a comment or extract a named constant to explain why we divide by 10 (e.g. Darwin platform/WebSocketTask limitation) and consider exposing it via config or at least centralizing the factor:
@@ internal suspend fun execute(request: HttpRequestData, callContext: CoroutineContext): HttpResponseData { - val maxFrameSize = request.attributes[WEBSOCKETS_KEY].maxFrameSize - task.setMaximumMessageSize(maxFrameSize / 10) + val maxFrameSize = request.attributes[WEBSOCKETS_KEY].maxFrameSize + // Darwin NSURLSessionWebSocketTask treats message size differently; + // divide by 10 to work around platform buffer limits + val darwinScaleFactor = 10 + task.setMaximumMessageSize(maxFrameSize / darwinScaleFactor)
| @Test | ||
| fun testWebSocketMaxFrameSize() = testClient { | ||
| config { | ||
| install(WebSockets) { | ||
| maxFrameSize = 100 | ||
| } | ||
| } | ||
|
|
||
| val shortMessage = "abc".repeat(5) | ||
| val longMessage = "def".repeat(500) | ||
| test { client -> | ||
| assertFailsWith<DarwinHttpRequestException> { | ||
| client.webSocket("$TEST_WEBSOCKET_SERVER/websockets/echo") { | ||
| send(shortMessage) | ||
| assertEquals(shortMessage, (incoming.receive() as Frame.Text).readText()) | ||
| send(longMessage) | ||
| val frame = incoming.receive() as Frame.Text | ||
| assertEquals(longMessage, frame.readText()) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify test logic for frame size calculation
The test configures maxFrameSize = 100, but the Darwin engine divides this by 10, resulting in a 10-byte limit. However, the short message "abc".repeat(5) is 15 characters/bytes, which should exceed the limit but is expected to succeed.
Please verify the frame size calculation logic:
The test might need adjustment based on the actual frame size enforcement logic. Consider whether the units are bytes vs characters, or if the division by 10 is correctly applied.
🏁 Script executed:
#!/bin/bash
# Check if there are other tests or documentation about frame size units
rg -A 10 -B 5 "maxFrameSize.*=.*[0-9]" --type ktLength of output: 77
🏁 Script executed:
#!/bin/bash
# Locate all usages of `maxFrameSize` in Kotlin sources to inspect how it's applied
rg -n 'maxFrameSize' -g '*.kt' -C 5Length of output: 75532
🏁 Script executed:
#!/bin/bash
# Search for the `convert()` extension in the Darwin engine sources
rg -n 'convert(' -g 'ktor-client-darwin/darwin/src/**'Length of output: 123
🏁 Script executed:
#!/bin/bash
# Search for the `convert` extension implementation in Darwin engine sources
rg -F -n 'convert(' -g 'ktor-client-darwin/darwin/src/**/*.kt'Length of output: 213
Adjust DarwinEngineTest to account for 10× reduction in Darwin engine’s frame limit
The Darwin engine does:
task.setMaximumMessageSize(maxFrameSize / 10)so configuring maxFrameSize = 100 actually enforces a 10-byte limit. Your test’s shortMessage is 15 bytes and will fail under that rule. You need to either:
- Raise the configured
maxFrameSizeso that(configuredValue/10) ≥ 15(for example, usemaxFrameSize = 150→ 150/10=15), or - Shrink
shortMessageto at most 10 bytes to match the current behavior.
Locations to update:
- ktor-client/ktor-client-darwin/darwin/test/DarwinEngineTest.kt at line 267 (
maxFrameSize = …)
🤖 Prompt for AI Agents
In ktor-client/ktor-client-darwin/darwin/test/DarwinEngineTest.kt around lines
263 to 284, the test sets maxFrameSize to 100, but the Darwin engine enforces a
limit of maxFrameSize divided by 10, effectively 10 bytes. Since the
shortMessage is 15 bytes, the test will fail. Fix this by either increasing
maxFrameSize to at least 150 so that the effective limit is 15 bytes or reducing
shortMessage to 10 bytes or less to comply with the current limit.
| val task = session.webSocketTaskWithRequest(nativeRequest) | ||
| val response = delegate.read(task, callContext) | ||
| val maxFrameSize = request.attributes[WEBSOCKETS_KEY].maxFrameSize | ||
| task.setMaximumMessageSize(maxFrameSize / 10) |
There was a problem hiding this comment.
could you write a comment behind the magic number?
bjhham
force-pushed
the
bjhham/ios-max-frame-size
branch
from
211a55c to
e0b31a5
Compare
|
please check API dump before merging |
bjhham
force-pushed
the
bjhham/ios-max-frame-size
branch
2 times, most recently
from
cba9772 to
556c10f
Compare
bjhham
force-pushed
the
bjhham/ios-max-frame-size
branch
from
556c10f to
72599e2
Compare
2 participants
Subsystem
Client, iOS, Websockets
Motivation
KTOR-6963 Darwin: The
maxFrameSizeoption has no effectSolution
We were assuming that we could set
maxFrameSizeon the session after the upgrade, but for Darwin, the property needs to be assigned beforeNSURLSessionWebSocketTask.resume()is called. So to provide access to the config value, I included the plugin details as an attribute for WS requests.