feat(auth): fix ClientSideCredentialAccessBoundary race condition#12681
feat(auth): fix ClientSideCredentialAccessBoundary race condition#12681vverman wants to merge 1 commit intogoogleapis:mainfrom
Conversation
This change addresses a race condition in ClientSideCredentialAccessBoundaryFactory that occurred when multiple concurrent calls were made to generateToken. The fix involves: - Waiting on the RefreshTask itself rather than its internal task. - Using a single listener in RefreshTask to ensure finishRefreshTask completes before the outer future unblocks waiting threads. - Adding a regression test generateToken_freshInstance_concurrent_noNpe.
There was a problem hiding this comment.
Code Review
This pull request refactors the RefreshTask in ClientSideCredentialAccessBoundaryFactory to use a single listener, ensuring internal state updates occur before the future completes. It also simplifies the refresh logic and adds a concurrent test case to address a race condition. Feedback was provided to enhance the robustness of error handling in the task listener, specifically by catching Throwable and handling null causes in ExecutionException to avoid potential hangs.
| } catch (ExecutionException e) { | ||
| Throwable cause = e.getCause(); | ||
| 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.")); | ||
| RefreshTask.this.setException(e.getCause()); | ||
| } catch (Exception e) { | ||
| RefreshTask.this.setException(e); | ||
| } |
There was a problem hiding this comment.
The error handling in this listener is less robust than the previous implementation. Specifically:
- Catching
Exceptioninstead ofThrowable: IffinishRefreshTaskorFutures.getDonethrows anError(e.g.,OutOfMemoryErrororNoClassDefFoundError), it will not be caught. This would cause the listener to terminate abruptly without completing theRefreshTaskfuture, leading to all threads waiting oncurrentRefreshTask.get()hanging indefinitely. - Potential
NullPointerException:RefreshTask.this.setException(e.getCause())will throw aNullPointerExceptionife.getCause()is null (which is technically possible for anExecutionException, though rare fromFuture.get()). SinceAbstractFuture.setExceptiondoes not allow null arguments, this would also cause the listener to crash and the future to hang.
Using Throwable and adding a null check for the cause ensures the future is always completed even in extreme failure scenarios.
} catch (ExecutionException e) {
Throwable cause = e.getCause();
RefreshTask.this.setException(cause != null ? cause : e);
} catch (Throwable t) {
RefreshTask.this.setException(t);
}
This change addresses a race condition in ClientSideCredentialAccessBoundaryFactory that occurred when multiple concurrent calls were made to generateToken.
The fix involves: