Skip to content

Conversation

@bittu9975
Copy link
Contributor

This PR resolves a race condition in AuthorizationRequestContext.loadRole, where multiple threads could execute the role initializer concurrently.
The fix introduces a FutureTask guarded by an AtomicReference, ensuring the initializer runs exactly once, even under concurrent access.

A focused unit test (TestAuthorizationRequestContext) is added to verify that loadRole executes the initializer only once when called from multiple threads.

@jerqi
Copy link
Contributor

jerqi commented Sep 30, 2025

This AuthorizationRequestContext won't be accessed in the multiple threads according to my understanding.

@bittu9975
Copy link
Contributor Author

@jerqi
You are right that, from the current usage, AuthorizationRequestContext would not be accessed by more than one thread.

My motivation for doing this change was to make loadRole() more future-proof and robust against future race conditions if the usage pattern changes (e.g., multi-threaded server environments or future functionality). The extra concurrency guard loads the role only once without incurring heavy overhead.

That being the case, if the maintainers prefer this extra complexity isn't required at the moment, I'd be content to just make it simpler.
Would you like me to:

Revert to the less complicated AtomicBoolean.compareAndSet guard, or

Maintain the present concurrency-safe solution for future security, perhaps with better documentation, or

Do something entirely different?

@jerqi
Copy link
Contributor

jerqi commented Sep 30, 2025

Context is usually used for provide the variables in the one thread. The context can be accessed by multiple thread. It is a little weird for me.

Copy link
Collaborator

@hdygxsj hdygxsj left a comment

Choose a reason for hiding this comment

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

What do you think about modifying it like this:

private final CompletableFuture<Void> roleLoadFuture = new CompletableFuture<>();
private final AtomicBoolean hasLoadedRole = new AtomicBoolean(false);

public void loadRole(Runnable runnable) {
    if (hasLoadedRole.compareAndSet(false, true)) {
        try {
            runnable.run();
            roleLoadFuture.complete(null);
        } catch (Throwable t) {
            roleLoadFuture.completeExceptionally(t);
        }
    }
}

public boolean authorizeAllow(...) {
    roleLoadFuture.join(); 
    AuthorizationKey context = new AuthorizationKey(...);
    return allowAuthorizerCache.computeIfAbsent(context, authorizer);
}

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