fix(internal-plugin-mercury): missing maxRetries property in mercury plugin config #3969
Conversation
WalkthroughThe pull request updates 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 (
|
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
k-wasniowski
added
the
validated
k-wasniowski
force-pushed
the
fix-do-cleanup-before-reconnect
branch
2 times, most recently
from
20f23b5
to
c798eba
Compare
… loop of mercury reconnection
k-wasniowski
force-pushed
the
fix-do-cleanup-before-reconnect
branch
from
c798eba
to
053265f
Compare
![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)
packages/@webex/internal-plugin-mercury/src/config.js (1)
40-44: Consider documenting retry behavior in logs.While the configuration is solid, it would be valuable for debugging and monitoring to ensure that retry attempts are properly logged.
Consider:
- Logging each retry attempt with current count
- Logging when max retries are exhausted
- Adding metrics/telemetry for retry-related events
packages/@webex/internal-plugin-mercury/README.md (1)
60-63: Consider enhancing the retry mechanism documentationWhile the parameters are documented, the documentation could be more helpful with:
- An explanation of how the exponential backoff works
- Examples of retry scenarios and timing
- Clarification of why
initialConnectionMaxRetriesdefaults to 0 whilemaxRetriesdefaults to 5Example addition:
- `maxRetries` - the number of times it will retry before error. Default: 5 - `initialConnectionMaxRetries` - the number of times it will retry before error on the first connection. Once a connection has been established, any further connection attempts will use `maxRetries`. Default: 0 - `backoffTimeMax` - The maximum time between connection attempts in ms. Default: 32000 - `backoffTimeReset` - The time before the first retry in ms. Default: 1000 + +### Retry Behavior Example + +When a connection fails, the system will: +1. Wait `backoffTimeReset` (1000ms) +2. Attempt reconnection +3. If failed, wait twice the previous wait time (exponential backoff) +4. Repeat until either: + - Connection succeeds + - `maxRetries` (5) is reached + - Wait time would exceed `backoffTimeMax` (32000ms)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/internal-plugin-mercury/README.md(1 hunks)packages/@webex/internal-plugin-mercury/src/config.js(1 hunks)
🔇 Additional comments (4)
packages/@webex/internal-plugin-mercury/src/config.js (2)
40-44: LGTM! The maxRetries configuration is well-implemented.
The implementation follows the established patterns in the file and effectively addresses the infinite reconnection loop issue. The default value of 5 retries provides a good balance between reliability and fail-fast behavior.
40-44: Verify integration with other timeout configurations.
The new maxRetries works in conjunction with existing backoff configurations. Let's verify the interaction.
✅ Verification successful
Let me gather more specific information about the retry and backoff implementation in Mercury.
Integration with timeout configurations verified successfully
The maxRetries configuration works correctly with the backoff strategy:
- It's properly integrated with the exponential backoff implementation using
backoff.ExponentialStrategy - Works in conjunction with
backoffTimeMax(32000ms) andbackoffTimeReset(1000ms) - The retry mechanism is well-tested, including edge cases for both initial connection and reconnection scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential interactions between retry logic and other timeout configurations
# Search for any retry-related logic that might interact with this configuration
rg -l 'retry|reconnect' --type js
# Look for backoff time usage in conjunction with connection attempts
ast-grep --pattern 'backoffTime(Max|Reset)'
Length of output: 2213
Script:
#!/bin/bash
# Check Mercury's implementation for retry and backoff logic
rg -A 10 "maxRetries" packages/@webex/internal-plugin-mercury/src/mercury.js
# Look for backoff time configuration and usage
rg "backoff" packages/@webex/internal-plugin-mercury/src/
# Check the test cases to understand the retry behavior
rg "maxRetries" packages/@webex/internal-plugin-mercury/test/
Length of output: 3641
packages/@webex/internal-plugin-mercury/README.md (2)
60-60: LGTM: maxRetries default value updated to prevent infinite loops
The change from unlimited retries to a maximum of 5 retries aligns well with the PR objective of preventing infinite reconnection loops.
60-63: Verify configuration parameters match implementation
Let's ensure the documented defaults match the actual implementation.
✅ Verification successful
Let me search for the other configuration parameters to complete the verification.
Documentation matches implementation
The documented configuration parameters and their default values are accurately reflected in the implementation:
maxRetries: Default 5 (config.js:process.env.MERCURY_MAX_RETRIES || 5)initialConnectionMaxRetries: Default 0 (not set in config, used conditionally in mercury.js)backoffTimeMax: Default 32000 (config.js:process.env.MERCURY_BACKOFF_TIME_MAX || 32000)backoffTimeReset: Default 1000 (config.js:process.env.MERCURY_BACKOFF_TIME_RESET || 1000)
All parameters are actively used in the codebase for connection retry logic and backoff timing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the configuration parameters in the source code
echo "Searching for maxRetries configuration..."
rg -A 5 "MERCURY_MAX_RETRIES|maxRetries" packages/@webex/internal-plugin-mercury/
echo "Searching for retry-related configuration..."
ast-grep --pattern 'const $_ = {
$$$
maxRetries: $_,
$$$
}'
Length of output: 5931
Script:
#!/bin/bash
# Search for backoff-related configuration
echo "Searching for backoff configuration..."
rg "backoffTimeMax|backoffTimeReset" packages/@webex/internal-plugin-mercury/
echo "Searching for initialConnectionMaxRetries..."
rg "initialConnectionMaxRetries" packages/@webex/internal-plugin-mercury/src/
Length of output: 3118
k-wasniowski
changed the title
COMPLETES #SPARK-548638
This pull request addresses
This PR addresses the issue that manifests itself as an infinite loop of reconnection. It occurs if during the meeting reconnection, JS-SDK is getting stuck on the mercury socket reconnection.
by making the following changes
Add missing configuration which specifies how many reconnection attempts we can do before giving up, otherwise we will try to connect infinitly.
Change Type
The following scenarios were tested
Tested with JS-SDK sample app, disconnecting client in various ways to identify if everytime, meeting will give up reconnections after 5 times.
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.
Summary by CodeRabbit
New Features
Documentation