[ECO-5163] fix: duplicated messages because of duplicated attach message #1052
Conversation
WalkthroughThe pull request introduces a new conditional check in the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🔇 Additional comments (2)lib/src/test/java/io/ably/lib/test/realtime/RealtimeMessageTest.java (2)
The attach listener implementation with timeout ensures proper channel attachment before proceeding with the test.
The test case thoroughly covers the duplicate messages scenario by:
Good practices observed:
Let's verify the test coverage for similar rewind scenarios: ✅ Verification successfulTest coverage for message duplication with rewind is adequate The test coverage for rewind functionality and message duplication is comprehensive:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for other rewind-related tests to ensure comprehensive coverage
# Search for rewind-related tests
echo "Searching for other rewind-related tests..."
rg -l "rewind.*test|test.*rewind" --type java
# Search for similar message duplication tests
echo "Searching for similar message duplication tests..."
rg -l "duplicate.*message|message.*duplicate" --type java
Length of output: 452 Script: #!/bin/bash
# Let's examine the content of these files to understand the test coverage
# Check RealtimeResumeTest.java for rewind-related tests
echo "=== Checking RealtimeResumeTest.java for rewind tests ==="
rg "rewind" -B 2 -A 2 lib/src/test/java/io/ably/lib/test/realtime/RealtimeResumeTest.java
# Check RealtimeMessageTest.java for other duplicate message scenarios
echo -e "\n=== Checking RealtimeMessageTest.java for duplicate message tests ==="
rg "duplicate.*message|message.*duplicate" -B 2 -A 2 lib/src/test/java/io/ably/lib/test/realtime/RealtimeMessageTest.java
# Let's also check for any rewind-related parameter usage
echo -e "\n=== Checking for rewind parameter usage ==="
rg '"rewind"' -B 2 -A 2 --type java
Length of output: 2447 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 (
|
github-actions
bot
temporarily deployed
to
staging/pull/1052/features
github-actions
bot
temporarily deployed
to
staging/pull/1052/javadoc
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/RealtimeMessageTest.java (1)
1019-1053: LGTM! Well-structured test case for duplicate message prevention.The test effectively validates that messages are not duplicated by:
- Publishing messages via REST client
- Subscribing via Realtime client with rewind
- Verifying exact message count (3) both immediately and after delay
Consider adding assertions for message content/order to ensure not only the count but also the integrity of messages:
channel.subscribe(message -> { int value = counter.incrementAndGet(); + assertEquals("Verify message content", + "message " + value, + message.data); if (value == 3) completionWaiter.onSuccess(); });lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
246-252: LGTM! Implementation aligns with RTL4i specification.The added condition correctly prevents duplicate attach messages by:
- Checking for connecting/disconnected connection states
- Setting channel state to attaching without sending request
- Returning early to avoid duplicate messages
Consider adding debug logging to help track these early returns:
if (connectionManager.getConnectionState().state == ConnectionState.connecting || connectionManager.getConnectionState().state == ConnectionState.disconnected) { + Log.v(TAG, String.format("attachImpl(): early return for channel = %s; connection state = %s", + name, connectionManager.getConnectionState().state)); setState(ChannelState.attaching, null); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeMessageTest.java(2 hunks)
🔇 Additional comments (1)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
246-252: Verify similar connection state checks in other message-sending methods.
Let's ensure consistent handling of connection states across all message-sending methods.
✅ Verification successful
Let's try a different approach using ripgrep to find these patterns with more context. The previous ast-grep patterns might have been too specific or not matching the actual syntax used in the codebase.
Based on the search results, I can now provide a final response regarding the verification of connection state checks in message-sending methods:
Connection state checks are consistently implemented across message-sending operations
The codebase shows consistent handling of connection states:
- In
ChannelBase.java, publish operations check connection state before sending messages - The Presence implementation verifies channel state before sending presence messages
- The attach operation (RTL4i) properly handles connecting/disconnected states
- All message sending operations use
connectionManager.send()with appropriate queueing behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other message-sending methods that might need similar connection state checks
# Look for methods that send protocol messages
ast-grep --pattern 'connectionManager.send($message, $_, $_)'
# Look for other attach implementations
ast-grep --pattern 'Action.attach'
Length of output: 98
Script:
#!/bin/bash
# Search for connection state checks and message sending patterns with broader context
rg -U "connectionManager.*send|getConnectionState" -A 5 -B 5
# Search for attach-related code
rg -U "Action\.attach|setState.*attaching" -A 5 -B 5
Length of output: 29352
|
Also, it seems some of the realtime tests failing. |
ttypic
force-pushed
the
ECO-5163/fix-duplicate-messages
branch
from
198e003 to
6a44cf9
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1052/features
github-actions
bot
temporarily deployed
to
staging/pull/1052/javadoc
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeMessageTest.java (1)
1024-1028: Consider improving test data clarity.The test messages could be more descriptive to better indicate their purpose in the test case.
- List<String> testMessagesData = List.of("message 1", "message 2", "message 3"); + List<String> testMessagesData = List.of( + "rewind_test_message_1", + "rewind_test_message_2", + "rewind_test_message_3" + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeMessageTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
🔇 Additional comments (2)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeMessageTest.java (2)
16-18: LGTM!
The new imports are appropriate for the functionality needed in the new test method.
1019-1051: LGTM! Well-structured test implementation.
The test effectively verifies the fix for duplicated messages by:
- Publishing messages via REST client
- Subscribing with rewind option via Realtime client
- Verifying exact message count
- Including delay to check for duplicates
Good practices used:
- Try-with-resources for proper resource cleanup
- CompletionWaiter for handling async operations
- AtomicInteger for thread-safe counting
- Unique channel name to avoid test interference
ttypic
force-pushed
the
ECO-5163/fix-duplicate-messages
branch
from
6a44cf9 to
02fde8c
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1052/features
github-actions
bot
temporarily deployed
to
staging/pull/1052/javadoc
ttypic
force-pushed
the
ECO-5163/fix-duplicate-messages
branch
from
02fde8c to
97f7c1c
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1052/features
github-actions
bot
temporarily deployed
to
staging/pull/1052/javadoc
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeMessageTest.java (1)
1016-1053: Well-structured test implementation!The test effectively verifies the fix for duplicated messages by:
- Publishing messages via REST API
- Subscribing via Realtime with rewind parameter
- Verifying message count both immediately and after a delay
A few suggestions to enhance the test:
- Consider using
List.of()instead of array initialization for cleaner message creation- Consider adding assertions for message content/order verification
- Message[] messages = new Message[] { - new Message("name", "message 1"), - new Message("name", "message 2"), - new Message("name", "message 3"), - }; + Message[] messages = List.of( + new Message("name", "message 1"), + new Message("name", "message 2"), + new Message("name", "message 3") + ).toArray(new Message[0]); channel.publish(messages); } try (AblyRealtime realtime = new AblyRealtime(opts)) { final ChannelOptions options = new ChannelOptions(); options.params = new HashMap<>(); options.params.put("rewind", "10"); final Channel channel = realtime.channels.get(testChannelName, options); final CompletionWaiter completionWaiter = new CompletionWaiter(); final AtomicInteger counter = new AtomicInteger(); + final List<String> receivedMessages = new ArrayList<>(); channel.subscribe(message -> { int value = counter.incrementAndGet(); + receivedMessages.add((String)message.data); if (value == 3) completionWaiter.onSuccess(); }); completionWaiter.waitFor(); assertEquals("Should be exactly 3 messages", 3, counter.get()); + assertArrayEquals("Messages should be received in order", + new String[]{"message 1", "message 2", "message 3"}, + receivedMessages.toArray(new String[0])); Thread.sleep(1500); assertEquals("Should be exactly 3 messages even after 1.5 sec wait", 3, counter.get()); + assertArrayEquals("Messages should remain in order after delay", + new String[]{"message 1", "message 2", "message 3"}, + receivedMessages.toArray(new String[0]));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeMessageTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java
- lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
🔇 Additional comments (1)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeMessageTest.java (1)
16-16: LGTM!
The added imports are necessary for the new test implementation.
Also applies to: 23-23
ttypic
force-pushed
the
ECO-5163/fix-duplicate-messages
branch
from
97f7c1c to
b5d7b45
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1052/features
github-actions
bot
temporarily deployed
to
staging/pull/1052/javadoc
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeMessageTest.java (1)
1016-1053: LGTM! Well-structured test case.The test effectively verifies that messages are not duplicated when using rewind option. It covers both the initial message receipt and ensures no duplicates arrive after a delay.
Consider adding assertions for message content and order to ensure not only the count but also the integrity of the messages:
channel.subscribe(message -> { int value = counter.incrementAndGet(); + assertEquals("message" + (value - 1), message.data); if (value == 3) completionWaiter.onSuccess(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(1 hunks)lib/src/main/java/io/ably/lib/transport/ConnectionManager.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeMessageTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java
- lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
🔇 Additional comments (2)
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (2)
1687-1694: LGTM! Fix prevents duplicate attach messages.
The implementation correctly prevents duplicate attach messages by:
- Filtering out attach messages from queued messages since they are always sent during connection events
- Using a clearer
!isEmpty()check for the loop condition
1687-1694: Verify message ordering and completeness.
While the fix for duplicate attach messages looks good, let's verify:
- Message ordering is preserved when filtering out attach messages
- No messages are lost during reconnection
- Other message types are processed correctly
✅ Verification successful
Let me gather more specific information about the message handling implementation and tests.
Based on the code implementation details revealed, I can now provide a final response:
Message handling implementation verified as safe
The implementation safely preserves message ordering and completeness:
- Messages are stored in an ordered queue (
queuedMessages) and processed sequentially - The
sendQueuedMessages()method maintains order by processing from index 0 - Attach message filtering happens during send without affecting queue order
- Message serial numbers are properly managed for pending messages
- Comprehensive error handling ensures no message is silently dropped
The changes to skip attach messages are implemented at the send phase only, after the message ordering is established, and don't interfere with the handling of other message types or the queue's integrity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for message ordering and handling tests
# Look for tests that verify message ordering and completeness
# Search for message ordering tests
echo "Searching for message ordering tests..."
rg -l "message.*order" --type java
# Search for message handling during reconnection
echo "Searching for reconnection message handling tests..."
rg -l "reconnect.*message" --type java
# Search for attach message handling
echo "Searching for attach message handling..."
rg -l "attach.*message" --type java
Length of output: 1489
Script:
#!/bin/bash
# Check implementation of message ordering and handling in ConnectionManager
echo "Checking message queue implementation..."
rg -A 10 "QueuedMessage" lib/src/main/java/io/ably/lib/transport/ConnectionManager.java
# Check tests for reconnection and message handling
echo "Checking reconnection tests..."
rg -A 10 "testReconnect.*message" lib/src/test/java/io/ably/lib/test/realtime/RealtimeResumeTest.java
# Check attach message handling tests
echo "Checking attach message tests..."
rg -A 10 "testAttach" lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java
# Check message ordering verification in tests
echo "Checking message ordering verification..."
rg -A 5 "verify.*message.*order" lib/src/test/java/io/ably/lib/test/realtime/RealtimeResumeTest.java
Length of output: 12418
|
If this is a sandbox issue, I will approve PR immediately ( since |
ttypic
force-pushed
the
ECO-5163/fix-duplicate-messages
branch
from
b5d7b45 to
f653d2d
Compare
Resolves #1050
Implemented missing (RTL4i) spec point to prevent duplicated attach message
Summary by CodeRabbit
New Features
Bug Fixes
Tests