Repository navigation
fix: report error code 80017 for a closed connection - #1252
RaphaelFakhri wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe closed connection reason now uses the message "Connection closed", status 400, and error code 80017. A test checks those values in the closed state and when attaching a channel. ChangesClosed Connection Error
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change aligns the reported closed-connection error with the intended code and status. It is ready to merge after normal checks. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: reporting error code 80017 for a closed connection. Linked Issues check Issue [ #797] requires correction of the invalid10000code forREASON_CLOSED. The pull request changesConnectionManager.REASON_CLOSEDfrom `ErrorInfo("Can't attach when not in an active state",…Out of Scope Changes check The pull request changes one connection error declaration and adds one focused regression test. Both changes directly support issue [ #797]. No unrelated production behavior or unrelated test changes a…
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
🛠️ Fix failing CI checks 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checks the closed-state code,
And hops along the channel road.
Four hundred marks the status clear,
Eight-zero-zero-one-seven appears.
The test confirms the reason true,
Then nibbles clover in the queue.
Comment @coderabbitai help to get the list of available commands.
There was a problem hiding this comment.
🟡 Changes recommended
Update the test to catch and assert the synchronous attach exception.
1 open finding
What changed in this PR
Updates closed-connection errors to use error code 80017 and HTTP status 400.
Changes:
- Updated the shared closed-connection error.
- Added regression coverage for closure and subsequent channel attachment.
| File | Summary |
|---|---|
lib/src/test/java/io/ably/lib/transport/ConnectionManagerClosedErrorTest.java |
Tests closed-connection error details; the attach assertion currently aborts because attach() throws synchronously. |
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java |
Uses the corrected closed-connection error details. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ably.channels.get("closed_connection_attach").attach(new CompletionListener() { | ||
| @Override | ||
| public void onSuccess() {} | ||
|
|
||
| @Override | ||
| public void onError(ErrorInfo reason) { | ||
| attachError.set(reason); | ||
| } | ||
| }); |
Fixes #797
Description
When the connection is
closingorclosed, the SDK reportsErrorInfo("Can't attach when not in an active state", 200, 10000). Error code10000means "no error" in the Ably error list, and the HTTP status code is200. This error appears in two places:reasonof theclosingandclosedconnection state changes, and of thedetachedchannel state change that follows a close.attach()on a channel after the connection closes.This change reports
ErrorInfo("Connection closed", 400, 80017)instead. Code80017is the "connection closed" code in the Ably error list, and ably-js reports the same code and status code for theclosingandclosedstates.Testing
ConnectionManagerClosedErrorTestconnects through a mock transport that refuses connections, closes the connection, and assertscode80017 andstatusCode400 on theclosedstate change and on a laterattach(). It fails without the change (Expected: is <80017> but: was <10000>) and passes with it.Run it with:
Summary by CodeRabbit