[ECO-4972] Fix implicit channel ATTACH #1043
Conversation
github-actions
bot
temporarily deployed
to
staging/pull/1043/features
github-actions
bot
temporarily deployed
to
staging/pull/1043/javadoc
sacOO7
force-pushed
the
fix/RTP6e-implicit-channel-attach
branch
from
6c328bc to
a0354e5
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1043/features
sacOO7
changed the title
github-actions
bot
temporarily deployed
to
staging/pull/1043/javadoc
WalkthroughThis pull request includes updates to documentation comments for the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (4)
lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java (1)
1687-1727: LGTM: Test for exception when subscribing with completion listener and attachOnSubscribe=falseThis new test method correctly verifies that an exception is thrown when attempting to subscribe with a completion listener when attachOnSubscribe is set to false. The test checks for the correct error code, message, and channel state.
However, there's a minor improvement that could be made:
Consider adding an assertion to check that the channel state remains initialized after the exception is caught. This would further confirm that no implicit attach occurred.
} catch (AblyException e) { assertEquals(400, e.errorInfo.statusCode); assertEquals(40000, e.errorInfo.code); assertThat(e.errorInfo.message, containsString("attachOnSubscribe=false doesn't expect attach completion callback")); } assertEquals(ChannelState.initialized, channel.state); +assertEquals("Verify channel state remains initialized after exception", ChannelState.initialized, channel.state);lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java (3)
Line range hint
918-918: Correct the assertion message to match the verified stateThe assertion message on line 918 reads "Verify detaching state reached", but it's verifying that the channel state is
ChannelState.attaching. To avoid confusion, please update the message to accurately reflect the state being checked.Apply this diff to fix the assertion message:
- assertEquals("Verify detaching state reached", ChannelState.attaching, channel.state); + assertEquals("Verify attaching state reached", ChannelState.attaching, channel.state);
Line range hint
954-987: Ensure consistency in variable namingIn the method
detach_when_channel_in_attaching_state, consider renaming the variablechannelNametotestChannelNamefor clarity and consistency with other test methods.
Line range hint
1194-1258: Include exception details in failure messages for better debuggingIn the catch block for
AblyException, enhance the failure message by including the exception details. This provides more context when the test fails unexpectedly.Apply this diff to improve the failure message:
- fail("channel_reattach_failed_timeout: unexpected exception"); + fail("channel_reattach_failed_timeout: unexpected exception: " + e.getMessage());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1 hunks)
- lib/src/main/java/io/ably/lib/realtime/Presence.java (1 hunks)
- lib/src/main/java/io/ably/lib/types/ChannelOptions.java (1 hunks)
- lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java (1 hunks)
- lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java (3 hunks)
✅ Files skipped from review due to trivial changes (2)
- lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
- lib/src/main/java/io/ably/lib/types/ChannelOptions.java
🧰 Additional context used
🔇 Additional comments (7)
lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java (2)
1653-1655: LGTM: Subscription without implicit attachThe implementation correctly tests the subscription without implicit attach by verifying that the channel state remains initialized after subscribing.
1656-1657: LGTM: Subscription for specific action without implicit attachThis test case verifies that subscribing to a specific action (Action.enter) also doesn't trigger an implicit attach.
lib/src/main/java/io/ably/lib/realtime/Presence.java (2)
300-302: Documentation update looks goodThe updated JavaDoc comments provide clear information about the method and its parameters.
305-312: Improved error handling forattachOnSubscribeEnabledThe added check for
attachOnSubscribeEnabledand proper handling when acompletionListeneris provided enhances the robustness of the method.lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java (3)
Line range hint
1079-1192: Thechannel_attach_retry_failedtest method effectively simulates network failuresThe implementation correctly tests the channel's retry logic and state transitions during network interruptions.
Line range hint
1260-1342: Thechannel_reattach_failed_errortest method correctly handles server error scenariosThis test appropriately simulates server-initiated detachments with errors and verifies that the channel transitions to the suspended state as expected.
Line range hint
399-435: Review synchronization when accessingreceivedMsgIn the
subscribe_without_implicit_attachmethod, whilereceivedMsgis a synchronized list, ensure that all access to it is thread-safe. The size check within the lambda expression may not be thread-safe without proper synchronization.Consider using a thread-safe data structure like
ConcurrentLinkedQueueor synchronizing the access when checkingreceivedMsg.size(). Run the following script to verify thread safety concerns:✅ Verification successful
Thread-safe access to
receivedMsgconfirmedAccess to
receivedMsg.size()within thesubscribe_without_implicit_attachmethod is thread-safe asreceivedMsgis aCollections.synchronizedList. No additional synchronization is required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of Collections.synchronizedList in the codebase # Test: Find instances where Collections.synchronizedList is used rg --type java 'Collections\.synchronizedList'Length of output: 541
Script:
#!/bin/bash # Description: Search for usages of receivedMsg.size() in RealtimeChannelTest.java rg 'receivedMsg\.size\(\)' lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.javaLength of output: 274
Script:
#!/bin/bash # Description: Find the implementation of the wait method in Helpers.ConditionalWaiter rg 'public .* wait\(' lib/src/test/java/io/ably/lib/test/common/Helpers.javaLength of output: 159
Script:
#!/bin/bash # Description: Extract the full implementation of the wait method in Helpers.ConditionalWaiter rg -A 20 'public .* wait\(' lib/src/test/java/io/ably/lib/test/common/Helpers.javaLength of output: 931
lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java (2)
1653-1656: Subscription calls look good, but consider adding assertions for channel state.The changes to the
presence_subscribe_without_implicit_attachmethod look correct. The subscription calls have been simplified by removing the completion listener. However, to strengthen the test, consider adding assertions to verify the channel state immediately after each subscription call.You could add assertions like this after each subscription call:
assertEquals(ChannelState.initialized, channel.state);This would explicitly verify that the channel remains in the initialized state after each subscription.
1687-1721: New test method looks good, consider adding a positive test case.The new test method
presence_subscribe_without_implicit_attach_and_completion_listener_throws_exceptionis well-implemented. It correctly tests the scenario where an exception should be thrown when a completion listener is passed with attachOnSubscribe set to false.To make the test more comprehensive, consider adding a positive test case where the subscription succeeds without a completion listener. This could be done by adding another test method or by extending this method to include both scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- lib/src/main/java/io/ably/lib/realtime/Presence.java (1 hunks)
- lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/src/main/java/io/ably/lib/realtime/Presence.java
🧰 Additional context used
🔇 Additional comments (1)
lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java (1)
Line range hint
1-3063: Overall, the changes improve test coverage for presence subscription behavior.The modifications to the existing test method and the addition of the new test method enhance the test suite's coverage of presence subscription behavior, particularly in scenarios involving implicit channel attachment and completion listeners. The changes are well-implemented and align with the test class's overall structure and style.
presence.subscribebased on updated spec -> https://github.com/ably/specification/pull/209/files, if completionListener is passed, it should throw exception.message.subscribedoesn't accept optionalcompletionListener, so no need to update spec for the same.attachOnSubscribechannel option (TB4) #1027Summary by CodeRabbit
New Features
Bug Fixes
Tests