Revert "fix(auth): Address ClientSideCredentialAccessBoundary RefreshTask race condition"#12770
Revert "fix(auth): Address ClientSideCredentialAccessBoundary RefreshTask race condition"#12770
Conversation
…Task rac…" This reverts commit 30088d2.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the task synchronization logic in ClientSideCredentialAccessBoundaryFactory and removes a concurrency regression test. The review feedback indicates that reverting to currentRefreshTask.task.get() and splitting the completion logic into separate listeners re-introduces race conditions, as there is no guarantee that the factory state updates before waiters are unblocked. Furthermore, a potential NullPointerException was identified in the new exception handling code, and the removal of the regression test is discouraged.
| try { | ||
| // Wait for the refresh task to complete. | ||
| currentRefreshTask.get(); | ||
| currentRefreshTask.task.get(); |
There was a problem hiding this comment.
Reverting this to wait on currentRefreshTask.task.get() instead of currentRefreshTask.get() re-introduces a race condition. The internal task completes as soon as the network request is done, but before the factory state has been updated via finishRefreshTask. This allows concurrent calls to proceed with potentially stale or incomplete state.
| currentRefreshTask.task.get(); | |
| currentRefreshTask.get(); |
| // Add listener to update factory's credentials when the task completes. | ||
| task.addListener( | ||
| () -> { | ||
| try { | ||
| finishRefreshTask(task); | ||
| RefreshTask.this.set(Futures.getDone(task)); | ||
| } catch (ExecutionException e) { | ||
| Throwable cause = e.getCause(); | ||
| RefreshTask.this.setException(cause != null ? cause : e); | ||
| } catch (Throwable t) { | ||
| RefreshTask.this.setException(t); | ||
| RefreshTask.this.setException(cause); | ||
| } | ||
| }, | ||
| MoreExecutors.directExecutor()); | ||
|
|
||
| // Add callback to set the result or exception based on the outcome. | ||
| Futures.addCallback( | ||
| task, | ||
| new FutureCallback<IntermediateCredentials>() { | ||
| @Override | ||
| public void onSuccess(IntermediateCredentials result) { | ||
| RefreshTask.this.set(result); | ||
| } | ||
|
|
||
| @Override | ||
| public void onFailure(@Nullable Throwable t) { | ||
| RefreshTask.this.setException( | ||
| t != null ? t : new IOException("Refresh failed with null Throwable.")); | ||
| } | ||
| }, | ||
| MoreExecutors.directExecutor()); |
There was a problem hiding this comment.
This implementation re-introduces a race condition by using two separate listeners. There is no guarantee that finishRefreshTask (which updates the factory state) will execute before RefreshTask.this.set(result) (which unblocks waiters). Additionally, line 505 is not null-safe and will throw a NullPointerException if cause is null. It is highly recommended to use a single listener to ensure correct sequencing.
// Single listener to guarantee that finishRefreshTask updates the internal state BEFORE
// the outer future completes and unblocks waiters.
task.addListener(
() -> {
try {
finishRefreshTask(task);
RefreshTask.this.set(Futures.getDone(task));
} catch (ExecutionException e) {
Throwable cause = e.getCause();
RefreshTask.this.setException(cause != null ? cause : e);
} catch (Throwable t) {
RefreshTask.this.setException(t);
}
},
MoreExecutors.directExecutor());| "Expected zero NullPointerExceptions due to the race condition, but some were thrown."); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The removal of the generateToken_freshInstance_concurrent_noNpe regression test is discouraged. This test was specifically designed to catch the race condition that is being re-introduced by this revert. Keeping the test ensures that the concurrency issue is properly tracked and eventually fixed without regression.
|
Not proceed with the revert. Original PR is fine. |
Pull request was closed
1 participant
Reverts #12681