fix: persist preferred host updates correctly in HttpScheduler retry logic#1206
Open
fix: persist preferred host updates correctly in HttpScheduler retry logic#1206
HttpScheduler retry logic#1206Conversation
…y logic - Prevent successful response from re-establish fallback host as the preference after fallback timeout - Ensured the preferred host is only updated when necessary during retry attempts
WalkthroughThis change modifies host fallback pinning logic in the HTTP scheduler to only persist a successful fallback host when it differs from the original preferred host, and adds a test verifying that late-arriving responses don't trigger unwanted re-pinning after the fallback retry timeout expires. Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/src/main/java/io/ably/lib/http/HttpScheduler.java`:
- Around line 199-210: The current repinning logic compares candidateHost to the
preferredHost captured at request start (preferredHost / candidateHost) which
can incorrectly repin if the original pin expired during the request; change the
check before calling httpCore.hosts.setPreferredHost(candidateHost, true) to
verify the current preferred host and pin age/validity instead of the
start-of-request string — fetch the live preferred host and its pin expiry/age
from httpCore.hosts (or add an accessor like
getPreferredHostExpiry/isPreferredHostPinned) after httpExecuteWithRetry
returns, and only call httpCore.hosts.setPreferredHost(candidateHost, true) if
candidateHost differs from the current live preferred host and the existing pin
is expired/older than fallbackRetryTimeout (or otherwise invalid). Ensure the
change touches the block that calls httpExecuteWithRetry(...) and the subsequent
setPreferredHost(...) call so persistence is gated on current pin validity
rather than the initial preferredHost snapshot.
In `@lib/src/test/java/io/ably/lib/test/rest/HttpTest.java`:
- Around line 1479-1500: The test creates an ExecutorService named executor for
requestFuture but never guarantees shutdown on failures; wrap the code that uses
executor/ requestFuture/ delayedRequestStarted in a try { ... } finally { ... }
block and call executor.shutdownNow() (or shutdown() then awaitTermination) in
the finally to ensure the single-thread executor is always cleaned up; keep
existing assertions and requestFuture.get() inside the try and perform executor
shutdown/termination in the finally to avoid stray test threads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 086a75d9-c331-4a12-974e-92f411409ff8
📒 Files selected for processing (2)
lib/src/main/java/io/ably/lib/http/HttpScheduler.javalib/src/test/java/io/ably/lib/test/rest/HttpTest.java
Comment on lines
+199
to
+210
| String preferredHost = httpCore.hosts.getPreferredHost(); | ||
| String candidateHost = preferredHost; | ||
| int retryCountRemaining = (httpCore.hosts.fallbackHostsRemaining(candidateHost) > 0) ? httpCore.options.httpMaxRetryCount : 0; | ||
|
|
||
| while(!isCancelled) { | ||
| try { | ||
| boolean shouldPersist = !candidateHost.equals(preferredHost); | ||
| result = httpExecuteWithRetry(candidateHost, path, requireAblyAuth); | ||
| setResult(result); | ||
| httpCore.hosts.setPreferredHost(candidateHost, true); | ||
| if (shouldPersist) { | ||
| httpCore.hosts.setPreferredHost(candidateHost, true); | ||
| } |
There was a problem hiding this comment.
Avoid repinning based only on the start-of-request snapshot.
This only compares the final candidateHost against the host captured at request start. A request that begins on a pinned fallback, retries onto another fallback, and completes after fallbackRetryTimeout can still re-pin a fallback host here, which brings back the behavior this PR is trying to remove.
Consider carrying the pin expiry/age through the request or checking whether the pin is still valid at completion time, rather than keying persistence off the initial host string alone.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/src/main/java/io/ably/lib/http/HttpScheduler.java` around lines 199 -
210, The current repinning logic compares candidateHost to the preferredHost
captured at request start (preferredHost / candidateHost) which can incorrectly
repin if the original pin expired during the request; change the check before
calling httpCore.hosts.setPreferredHost(candidateHost, true) to verify the
current preferred host and pin age/validity instead of the start-of-request
string — fetch the live preferred host and its pin expiry/age from
httpCore.hosts (or add an accessor like
getPreferredHostExpiry/isPreferredHostPinned) after httpExecuteWithRetry
returns, and only call httpCore.hosts.setPreferredHost(candidateHost, true) if
candidateHost differs from the current live preferred host and the existing pin
is expired/older than fallbackRetryTimeout (or otherwise invalid). Ensure the
change touches the block that calls httpExecuteWithRetry(...) and the subsequent
setPreferredHost(...) call so persistence is gated on current pin validity
rather than the initial preferredHost snapshot.
Comment on lines
+1479
to
+1500
| ExecutorService executor = Executors.newSingleThreadExecutor(); | ||
| Future<Long> requestFuture = executor.submit(() -> { | ||
| try { return client.time(); } | ||
| catch (AblyException e) { throw new RuntimeException(e); } | ||
| }); | ||
|
|
||
| // Wait until req3 has actually entered the listener before starting the clock | ||
| assertTrue("Delayed request must start within 5 s", delayedRequestStarted.await(5, TimeUnit.SECONDS)); | ||
|
|
||
| // Wait 150 ms so that fallbackRetryTimeout (100 ms) expires | ||
| Thread.sleep(150L); | ||
|
|
||
| // req4: timeout expired → primary tried again | ||
| client.time(); | ||
|
|
||
| // Wait for req3's delayed response to arrive (late fallback success) | ||
| requestFuture.get(5, TimeUnit.SECONDS); | ||
|
|
||
| // req5: late success from req3 must NOT have re-pinned the fallback | ||
| client.time(); | ||
|
|
||
| executor.shutdown(); |
There was a problem hiding this comment.
Shut down the executor in a finally block.
If await, get, or one of the assertions fails, this single-thread executor never gets cleaned up. That can leave stray test threads running and make failures noisier.
Suggested cleanup
- ExecutorService executor = Executors.newSingleThreadExecutor();
- Future<Long> requestFuture = executor.submit(() -> {
- try { return client.time(); }
- catch (AblyException e) { throw new RuntimeException(e); }
- });
+ ExecutorService executor = Executors.newSingleThreadExecutor();
+ try {
+ Future<Long> requestFuture = executor.submit(() -> {
+ try { return client.time(); }
+ catch (AblyException e) { throw new RuntimeException(e); }
+ });
- // Wait until req3 has actually entered the listener before starting the clock
- assertTrue("Delayed request must start within 5 s", delayedRequestStarted.await(5, TimeUnit.SECONDS));
+ // Wait until req3 has actually entered the listener before starting the clock
+ assertTrue("Delayed request must start within 5 s", delayedRequestStarted.await(5, TimeUnit.SECONDS));
- // Wait 150 ms so that fallbackRetryTimeout (100 ms) expires
- Thread.sleep(150L);
+ // Wait 150 ms so that fallbackRetryTimeout (100 ms) expires
+ Thread.sleep(150L);
- // req4: timeout expired → primary tried again
- client.time();
+ // req4: timeout expired → primary tried again
+ client.time();
- // Wait for req3's delayed response to arrive (late fallback success)
- requestFuture.get(5, TimeUnit.SECONDS);
+ // Wait for req3's delayed response to arrive (late fallback success)
+ requestFuture.get(5, TimeUnit.SECONDS);
- // req5: late success from req3 must NOT have re-pinned the fallback
- client.time();
+ // req5: late success from req3 must NOT have re-pinned the fallback
+ client.time();
+ } finally {
+ executor.shutdownNow();
+ }
-
- executor.shutdown();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ExecutorService executor = Executors.newSingleThreadExecutor(); | |
| Future<Long> requestFuture = executor.submit(() -> { | |
| try { return client.time(); } | |
| catch (AblyException e) { throw new RuntimeException(e); } | |
| }); | |
| // Wait until req3 has actually entered the listener before starting the clock | |
| assertTrue("Delayed request must start within 5 s", delayedRequestStarted.await(5, TimeUnit.SECONDS)); | |
| // Wait 150 ms so that fallbackRetryTimeout (100 ms) expires | |
| Thread.sleep(150L); | |
| // req4: timeout expired → primary tried again | |
| client.time(); | |
| // Wait for req3's delayed response to arrive (late fallback success) | |
| requestFuture.get(5, TimeUnit.SECONDS); | |
| // req5: late success from req3 must NOT have re-pinned the fallback | |
| client.time(); | |
| executor.shutdown(); | |
| ExecutorService executor = Executors.newSingleThreadExecutor(); | |
| try { | |
| Future<Long> requestFuture = executor.submit(() -> { | |
| try { return client.time(); } | |
| catch (AblyException e) { throw new RuntimeException(e); } | |
| }); | |
| // Wait until req3 has actually entered the listener before starting the clock | |
| assertTrue("Delayed request must start within 5 s", delayedRequestStarted.await(5, TimeUnit.SECONDS)); | |
| // Wait 150 ms so that fallbackRetryTimeout (100 ms) expires | |
| Thread.sleep(150L); | |
| // req4: timeout expired → primary tried again | |
| client.time(); | |
| // Wait for req3's delayed response to arrive (late fallback success) | |
| requestFuture.get(5, TimeUnit.SECONDS); | |
| // req5: late success from req3 must NOT have re-pinned the fallback | |
| client.time(); | |
| } finally { | |
| executor.shutdownNow(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/src/test/java/io/ably/lib/test/rest/HttpTest.java` around lines 1479 -
1500, The test creates an ExecutorService named executor for requestFuture but
never guarantees shutdown on failures; wrap the code that uses executor/
requestFuture/ delayedRequestStarted in a try { ... } finally { ... } block and
call executor.shutdownNow() (or shutdown() then awaitTermination) in the finally
to ensure the single-thread executor is always cleaned up; keep existing
assertions and requestFuture.get() inside the try and perform executor
shutdown/termination in the finally to avoid stray test threads.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Tests