MLE-27388: Refactor: Renamed the "checkFirstRequest" stuff#1915
MLE-27388: Refactor: Renamed the "checkFirstRequest" stuff#1915
Conversation
|
Copyright Validation Results ✅ Valid Files
✅ All files have valid copyright headers! |
There was a problem hiding this comment.
Pull request overview
Refactors OkHttp retry/“first request” logic naming to better reflect digest-auth ping behavior, and centralizes retry sleep interruption handling inside RetryContext.
Changes:
- Renames
checkFirstRequestconcept touseDigestAuthPingand renames related helper methods to clarify intent (ping before streaming). - Moves
InterruptedExceptionhandling for retry backoff sleep intoRetryContext.sleepIfNeeded()and simplifies call sites. - Minor cleanup in
OkHttpServices(e.g., removing a local suppression line).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
marklogic-client-api/src/main/java/com/marklogic/client/impl/RetryContext.java |
Centralizes retry sleep interruption handling and removes checked exception from sleepIfNeeded(). |
marklogic-client-api/src/main/java/com/marklogic/client/impl/OkHttpServices.java |
Renames and clarifies digest-auth pre-ping behavior and updates retry loops to use new RetryContext.sleepIfNeeded() signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
marklogic-client-api/src/main/java/com/marklogic/client/impl/RetryContext.java
Outdated
Show resolved
Hide resolved
marklogic-client-api/src/main/java/com/marklogic/client/impl/OkHttpServices.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
marklogic-client-api/src/main/java/com/marklogic/client/impl/OkHttpServices.java:535
- After
retryContext.sleepIfNeeded(), the thread may have been interrupted during the sleep (the interrupt flag is restored inRetryContext). The current code still proceeds to executedoFunction.apply(...), which can send another HTTP request even though cancellation/shutdown was requested. Consider checkingThread.currentThread().isInterrupted()after sleeping and aborting before issuing the next attempt.
for (; retryContext.shouldContinueRetrying(minRetryAttempts, maxDelayForRetries); retryContext.incrementRetry()) {
retryContext.sleepIfNeeded();
/*
* Execute the function which is passed as an argument
* in order to get the Response
*/
response = doFunction.apply(requestBldr);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
marklogic-client-api/src/main/java/com/marklogic/client/impl/OkHttpServices.java
Show resolved
Hide resolved
marklogic-client-api/src/main/java/com/marklogic/client/impl/OkHttpServices.java
Show resolved
Hide resolved
The new names actually convey what is going on as opposed to making you wonder what "check" might mean. Also modified sleepIfNeeded to catch the interrupted exception instead of repeating that catch.
5999019 to
f3366f8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
marklogic-client-api/src/main/java/com/marklogic/client/impl/OkHttpServices.java:540
retryContext.sleepIfNeeded()can return with the thread interrupt flag set (if interrupted during sleep), but the code will still proceed to invokedoFunction.apply(...)and send a request in this iteration. To honor cancellation, checkThread.currentThread().isInterrupted()immediately after sleeping and fail fast (e.g., throw a cancellation exception) before executing the request.
// If the thread is already interrupted, fail fast rather than attempting a request
if (Thread.currentThread().isInterrupted()) {
throw new MarkLogicIOException("Request cancelled: thread was interrupted before request could be sent");
}
RetryContext retryContext = new RetryContext(logger, RETRYABLE_STATUS_CODES, this::resetFirstRequestFlag);
Response response = null;
int status = -1;
/*
* This loop is for retrying the request if the service is unavailable
*/
for (; retryContext.shouldContinueRetrying(minRetryAttempts, maxDelayForRetries); retryContext.incrementRetry()) {
retryContext.sleepIfNeeded();
/*
* Execute the function which is passed as an argument
* in order to get the Response
*/
response = doFunction.apply(requestBldr);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Restore interrupt status for higher-level cancellation/shutdown logic | ||
| Thread.currentThread().interrupt(); |
There was a problem hiding this comment.
If the thread is interrupted during the retry sleep, sleepIfNeeded() restores the interrupt flag but the caller will still proceed to send the request in the same loop iteration. That can cause a request to be executed even though cancellation was requested. Consider having sleepIfNeeded() either propagate interruption (e.g., via a return value / exception) or have callers immediately check Thread.currentThread().isInterrupted() after sleeping and break/throw before doing any work.
| // Restore interrupt status for higher-level cancellation/shutdown logic | |
| Thread.currentThread().interrupt(); | |
| // Restore interrupt status and abort further retries | |
| Thread.currentThread().interrupt(); | |
| throw new FailedRetryException("Retry interrupted while waiting " + nextDelay + | |
| " ms before attempt " + retry); |
| int pingDelay = pingServer(callBaseUri, "", 0); | ||
| int pingRetries = 0; | ||
| int maxPingRetries = 10; // Prevent infinite loop | ||
| while (pingDelay > 0 && pingRetries < maxPingRetries && !Thread.currentThread().isInterrupted()) { | ||
| try { | ||
| Thread.sleep(pingDelay); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| throw new RuntimeException("Interrupted while waiting to ping server before streaming", e); | ||
| } | ||
| pingRetries++; | ||
| pingDelay = pingServer(callBaseUri, "", 0); | ||
| } |
There was a problem hiding this comment.
In the ping retry loop, pingServer(callBaseUri, "", 0) is called on every iteration with a constant retry value of 0, so calculateDelay(retry) never backs off across attempts. Consider passing pingRetries (or similar) into pingServer so delay increases with each ping attempt and aligns with the existing retry/backoff behavior elsewhere in this class.
| Thread.sleep(pingDelay); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| throw new RuntimeException("Interrupted while waiting to ping server before streaming", e); |
There was a problem hiding this comment.
Throwing a generic RuntimeException on InterruptedException here changes how interruption/cancellation is surfaced compared to the rest of this class (which typically throws MarkLogicIOException/FailedRequestException types). Consider throwing a consistent client exception type (and preserving the interrupt flag) so callers can reliably handle cancellation.
| throw new RuntimeException("Interrupted while waiting to ping server before streaming", e); | |
| throw new MarkLogicIOException("Interrupted while waiting to ping server before streaming", e); |
The new names actually convey what is going on as opposed to making you wonder what "check" might mean.
Also modified sleepIfNeeded to catch the interrupted exception instead of repeating that catch.