Skip to content

MLE-27388: Refactor: Renamed the "checkFirstRequest" stuff#1915

Merged
rjrudin merged 1 commit intodevelopfrom
feature/27388-refactor-checkRequest
Mar 3, 2026
Merged

MLE-27388: Refactor: Renamed the "checkFirstRequest" stuff#1915
rjrudin merged 1 commit intodevelopfrom
feature/27388-refactor-checkRequest

Conversation

@rjrudin
Copy link
Contributor

@rjrudin rjrudin commented Mar 3, 2026

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.

Copilot AI review requested due to automatic review settings March 3, 2026 16:24
@github-actions
Copy link

github-actions bot commented Mar 3, 2026

Copyright Validation Results
Total: 2 | Passed: 2 | Failed: 0 | Skipped: 0 | at: 2026-03-03 20:10:52 UTC | commit: f3366f8

✅ Valid Files

  • marklogic-client-api/src/main/java/com/marklogic/client/impl/OkHttpServices.java
  • marklogic-client-api/src/main/java/com/marklogic/client/impl/RetryContext.java

✅ All files have valid copyright headers!

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 checkFirstRequest concept to useDigestAuthPing and renames related helper methods to clarify intent (ping before streaming).
  • Moves InterruptedException handling for retry backoff sleep into RetryContext.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.

BillFarber
BillFarber previously approved these changes Mar 3, 2026
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 in RetryContext). The current code still proceeds to execute doFunction.apply(...), which can send another HTTP request even though cancellation/shutdown was requested. Consider checking Thread.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.

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.
@rjrudin rjrudin force-pushed the feature/27388-refactor-checkRequest branch from 5999019 to f3366f8 Compare March 3, 2026 20:10
@rjrudin rjrudin requested a review from Copilot March 3, 2026 20:10
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 invoke doFunction.apply(...) and send a request in this iteration. To honor cancellation, check Thread.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.

Comment on lines +51 to +52
// Restore interrupt status for higher-level cancellation/shutdown logic
Thread.currentThread().interrupt();
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// 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);

Copilot uses AI. Check for mistakes.
Comment on lines +5580 to +5592
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);
}
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Thread.sleep(pingDelay);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new RuntimeException("Interrupted while waiting to ping server before streaming", e);
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
throw new RuntimeException("Interrupted while waiting to ping server before streaming", e);
throw new MarkLogicIOException("Interrupted while waiting to ping server before streaming", e);

Copilot uses AI. Check for mistakes.
@rjrudin rjrudin merged commit bf0ae48 into develop Mar 3, 2026
9 checks passed
@rjrudin rjrudin deleted the feature/27388-refactor-checkRequest branch March 3, 2026 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants