KTOR-9411 Darwin: FrameTooBigException instead of generic exception#5453
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughMap Darwin EMSGSIZE websocket errors to FrameTooBigException; extend FrameTooBigException to accept a cause and implement CopyableThrowable; update WebSocket test to expect FrameTooBigException for oversized frames. ChangesWebSocket Frame Size Error Handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
osipxd
force-pushed
the
osipxd/darwin-frame-too-big
branch
from
f40d84a to
e4ecf01
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Preserve wrapped cause in FrameTooBigException.createCopy
Now that FrameTooBigException accepts a cause (used by Darwin via FrameTooBigException(..., DarwinHttpRequestException(error))), createCopy() should carry that cause forward. The current implementation rebuilds with only frameSize and then sets the copied exception’s cause to the original exception object, so after coroutine stacktrace recovery the immediate cause is no longer the original DarwinHttpRequestException. This breaks cause-based handling and makes diagnostics noisier when the exception crosses coroutine boundaries.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ktor-shared/ktor-websockets/common/src/io/ktor/websocket/FrameTooBigException.kt (1)
11-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the new public
causeparameter.The constructor surface changed here, but the KDoc still only describes
frameSize. Please addcauseso the public API docs stay accurate.As per coding guidelines, "Public API requires KDoc documentation including parameters, return values, and notable exceptions."
🤖 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-websockets/common/src/io/ktor/websocket/FrameTooBigException.kt` around lines 11 - 25, Update the KDoc for class FrameTooBigException to document the new public constructor parameter "cause": add an "@param cause" entry explaining that it's the optional underlying Throwable that caused this exception (nullable), and mention its role in exception chaining for the primary constructor of FrameTooBigException; ensure the existing "@param frameSize" text remains and that the deprecated secondary constructor comment need not change.
🤖 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-client/ktor-client-darwin/darwin/src/io/ktor/client/engine/darwin/internal/DarwinWebsocketSession.kt`:
- Around line 261-264: The Darwin mapping in convertWebsocketError currently
supplies a bogus negative frameSize (-1) to FrameTooBigException; instead make
the “unknown size” explicit: modify the convertWebsocketError handling to
construct FrameTooBigException with an explicit unknown-size representation
(e.g., pass null/Optional/absent size or a named constant like
UNKNOWN_FRAME_SIZE) rather than -1, and preserve wrapping with
DarwinHttpRequestException(error); update the call site in convertWebsocketError
and any FrameTooBigException constructors/usage to accept and display the
explicit unknown size representation.
---
Outside diff comments:
In
`@ktor-shared/ktor-websockets/common/src/io/ktor/websocket/FrameTooBigException.kt`:
- Around line 11-25: Update the KDoc for class FrameTooBigException to document
the new public constructor parameter "cause": add an "@param cause" entry
explaining that it's the optional underlying Throwable that caused this
exception (nullable), and mention its role in exception chaining for the primary
constructor of FrameTooBigException; ensure the existing "@param frameSize" text
remains and that the deprecated secondary constructor comment need not change.
🪄 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: 4abe8fad-dc06-402a-b5b4-e2ab57d4a4af
📒 Files selected for processing (3)
ktor-client/ktor-client-darwin/darwin/src/io/ktor/client/engine/darwin/internal/DarwinWebsocketSession.ktktor-client/ktor-client-tests/common/test/io/ktor/client/tests/WebSocketTest.ktktor-shared/ktor-websockets/common/src/io/ktor/websocket/FrameTooBigException.kt
osipxd
force-pushed
the
osipxd/darwin-frame-too-big
branch
from
e4ecf01 to
e2b353e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ktor-client/ktor-client-darwin/darwin/src/io/ktor/client/engine/darwin/internal/DarwinWebsocketSession.kt (1)
122-139: ⚡ Quick winApply
convertWebsocketErrorto send paths for consistent EMSGSIZE mapping.
sendMessagecompletion handlers (and the ping handler at Line 156) still wrap the rawNSErrorinDarwinHttpRequestExceptiondirectly. IfEMSGSIZEsurfaces while sending an oversized text/binary frame, callers will getDarwinHttpRequestExceptioninstead ofFrameTooBigException, which contradicts the stated PR objective for outbound oversized frames. Routing all NSError-to-Exception conversions throughconvertWebsocketErrorkeeps the mapping uniform.♻️ Proposed change
) { error -> if (error == null) { continuation.resume(Unit) } else { - continuation.resumeWithException(DarwinHttpRequestException(error)) + continuation.resumeWithException(convertWebsocketError(error)) } }Apply the same substitution at Line 138 (binary send) and Line 156 (ping/pong).
🤖 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-client/ktor-client-darwin/darwin/src/io/ktor/client/engine/darwin/internal/DarwinWebsocketSession.kt` around lines 122 - 139, The send paths and ping handler currently wrap NSError directly into DarwinHttpRequestException, so update the completion handlers for task.sendMessage (used in FrameType.TEXT and FrameType.BINARY branches) and the ping handler to call convertWebsocketError(error) instead of constructing DarwinHttpRequestException(error) so that EMSGIZE/oversize errors are mapped to FrameTooBigException; locate the send calls via task.sendMessage(...) and the ping callback in DarwinWebsocketSession and replace continuation.resumeWithException(DarwinHttpRequestException(error)) with continuation.resumeWithException(convertWebsocketError(error)) (or resumeWithException(convertWebsocketError(error))) to ensure uniform NSError→Exception conversion.
🤖 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-client/ktor-client-darwin/darwin/src/io/ktor/client/engine/darwin/internal/DarwinWebsocketSession.kt`:
- Around line 122-139: The send paths and ping handler currently wrap NSError
directly into DarwinHttpRequestException, so update the completion handlers for
task.sendMessage (used in FrameType.TEXT and FrameType.BINARY branches) and the
ping handler to call convertWebsocketError(error) instead of constructing
DarwinHttpRequestException(error) so that EMSGIZE/oversize errors are mapped to
FrameTooBigException; locate the send calls via task.sendMessage(...) and the
ping callback in DarwinWebsocketSession and replace
continuation.resumeWithException(DarwinHttpRequestException(error)) with
continuation.resumeWithException(convertWebsocketError(error)) (or
resumeWithException(convertWebsocketError(error))) to ensure uniform
NSError→Exception conversion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f08e855-cce7-493c-b210-f5affa18d952
📒 Files selected for processing (3)
ktor-client/ktor-client-darwin/darwin/src/io/ktor/client/engine/darwin/internal/DarwinWebsocketSession.ktktor-client/ktor-client-tests/common/test/io/ktor/client/tests/WebSocketTest.ktktor-shared/ktor-websockets/common/src/io/ktor/websocket/FrameTooBigException.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/WebSocketTest.kt
osipxd
force-pushed
the
osipxd/darwin-frame-too-big
branch
from
e2b353e to
9e5674f
Compare
osipxd
force-pushed
the
osipxd/darwin-frame-too-big
branch
from
9e5674f to
d637dd2
Compare
osipxd
force-pushed
the
osipxd/darwin-frame-too-big
branch
from
d637dd2 to
231d7d9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 231d7d9eab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @Deprecated("Maintained for binary compatibility", level = DeprecationLevel.HIDDEN) | ||
| public constructor(frameSize: Long) : this(frameSize, cause = null) |
There was a problem hiding this comment.
Marking the FrameTooBigException(Long) constructor as DeprecationLevel.HIDDEN makes that overload synthetic on JVM (as reflected in the updated ktor-websockets.api), so Java source code that previously compiled with new FrameTooBigException(size) will fail after this change unless it is rewritten to pass a second Throwable argument. This is a source-compatibility regression introduced by this commit and is likely unexpected for users upgrading within the same major line.
Useful? React with 👍 / 👎.
2 participants
Subsystem
Client Darwin
Motivation
KTOR-9411 Darwin throws DarwinHttpRequestException instead of FrameTooBigException
Solution
Add one more converter for NSError