KTOR-9361 Fix JsWebSocketSession._closeReason completed twice#5457
KTOR-9361 Fix JsWebSocketSession._closeReason completed twice#5457osipxd wants to merge 1 commit intorelease/3.xfrom
Conversation
Two bugs caused the issue: 1. In JsWebSocketSession.onClose, _closeReason.complete() was called unconditionally even when onError already called completeExceptionally(). Fixed by checking the return value: Frame.Close is only forwarded to _incoming when _closeReason was newly completed. 2. In awaitConnection(), the combined open/error event listener was never removed after the open event fired. When an error occurred on an established connection, the stale listener tried to resume the already- completed continuation, logging "Already resumed, but proposed with update". Fixed by using lateinit var so the listener can remove itself on first fire. Both fixes apply to js and wasmJs targets. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughWebSocket event handling in JavaScript and WASM client engines is improved by storing event callbacks in named variables for consistent identity, enabling proper listener removal. Close frame emission is now conditional on successful completion state completion, preventing redundant sends across both JavaScript and WASM implementations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![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
Here are some automated review suggestions for this pull request.
Reviewed commit: d5f766f2c9
ℹ️ 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".
| if (_closeReason.complete(reason)) { | ||
| _incoming.trySend(Frame.Close(reason)) | ||
| } |
There was a problem hiding this comment.
Conditioning _incoming.trySend(Frame.Close(reason)) on _closeReason.complete(reason) drops the peer's close frame whenever the client initiated shutdown first, because FrameType.CLOSE handling already completes _closeReason before websocket.close(...). In that common path, onClose now closes incoming without ever emitting Frame.Close, so callers that read incoming to observe the peer close reply (and its code/message) lose that signal; this regression also appears in the wasmJs counterpart.
Useful? React with 👍 / 👎.
2 participants
Subsystem
Client Js
Motivation
KTOR-9361 WebSockets:
JsWebSocketSession._closeReasonis completed twiceSolution
Two bugs caused the issue:
In
JsWebSocketSession.onClose,_closeReason.complete()was called unconditionally even whenonErroralready calledcompleteExceptionally(). Fixed by checking the return value:Frame.Closeis only forwarded to_incomingwhen_closeReasonwas newly completed.In
awaitConnection(), the combined open/error event listener was never removed after the open event fired. When an error occurred on an established connection, the stale listener tried to resume the already- completed continuation, logging "Already resumed, but proposed with update". Fixed by usinglateinit varso the listener can remove itself on first fire.Both fixes apply to js and wasmJs targets.