Hi,

I think the static AuthenticationManagerDelegator within AuthenticationConfiguration is never created at line 116, if I'm not mistaken.

First method call to AuthenticationConfiguration.getAuthenticationManager:

The flag authenticationManagerInitialized is false, and this.buildingAuthenticationManager.getAndSet(true) within the if statement at line 115 will return false, because the AtomicBoolean is initialized with false, and the getAndSet will return the old value, not the new one. Before returning at line 126, both flags authenticationManagerInitialized and buildingAuthenticationManager are now true.

Successive method calls to AuthenticationConfiguration.getAuthenticationManager:

The previously created AuthenticationManger will always be returned early at line 112, so there is no chance to reach the statement new AuthenticationManagerDelegator at line 116, right?

There is no actual bug I can refer to, but this looks like dead code which may indicate another problem. Maybe the AuthenticationManagerDelegator should be used under different circumstances? The documentation for AuthenticationManagerDelegator says:

Prevents infinite recursion in the event that initializing the AuthenticationManager.

Best regards, Daniel

Comment From: sjohnr

Thanks for reaching out @dru1,

I think the static AuthenticationManagerDelegator within AuthenticationConfiguration is never created at line 116, if I'm not mistaken.

The use of an AtomicBoolean indicates that the AuthenticationManagerDelegator likely exists to solve a race condition in multi-threaded access to the getAuthenticationManager() method. If that's the case, there is a possibility that in between the time when this.buildingAuthenticationManager.getAndSet(true) is called and this.authenticationManagerInitialized = true is set, another thread attempts to access the method. This is also confirmed by the name of the variable, which is buildingAuthenticationManager.

I don't know what specific instances that could occur in, but it would be fairly risky to attempt to refactor this code and remove usage without extensive tests to prove the issue doesn't exist anymore. While I haven't reviewed this deeply, I am fairly confident we don't want to make adjustments to this code such as removing that class. Make sense?

I'm going to close this issue but feel free to add more comments if you still have thoughts.