Cleanup Listener Handling in AuthorizationService #75252
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Two parts here: 1. Don't synchonize on the async supplier. Even though these aren't actually async yet it seeems, the synchronization wouldn't work anyway if they were. If this were to be called concurrently the current approach would cause blocking needlessly though. Either way, synchonizing ever call and all the downstream calls is just unnecessarily hard on the JIT here. 2. The async look over the interceptors was needlessly slow with all the synchronization etc. in the `StepListener`. Also it made the stacktraces on transport thread extremely deep and complicated. Simplified and made this faster by using a normal listener/iterator without any synchronization.
original-brownbear
added
>non-issue
:Security/Authorization
|
Pinging @elastic/es-security (Team:Security) |
| authorizedIndicesSupplier.getAsync( | ||
| ActionListener.wrap( | ||
| authorizedIndices -> | ||
| resolvedIndicesListener.onResponse( |
There was a problem hiding this comment.
inlined this to make the stack-trace one less deep and generally make the logic easier to follow, no need to have a method to resolve a listener when the method is blocking and one line anyway :)
tvernum
approved these changes
Jul 13, 2021
| listener.onResponse(value); | ||
| public void getAsync(ActionListener<V> listener) { | ||
| if (valueFuture == null) { | ||
| boolean addedListener = false; |
There was a problem hiding this comment.
Nit: addedListener doesn't seem to quite fit the usage here.
Suggested change
| boolean addedListener = false; | |
| boolean firstInvocation = false; |
ywangd
reviewed
Jul 13, 2021
Comment on lines
+377
to
+389
| final Iterator<RequestInterceptor> requestInterceptorIterator = requestInterceptors.iterator(); | ||
| requestInterceptorIterator.next().intercept(requestInfo, authorizationEngine, authorizationInfo, | ||
| new ActionListener.Delegating<>(listener) { | ||
| @Override | ||
| public void onResponse(Void unused) { | ||
| if (requestInterceptorIterator.hasNext()) { | ||
| requestInterceptorIterator.next().intercept(requestInfo, authorizationEngine, authorizationInfo, this); | ||
| } else { | ||
| listener.onResponse(null); | ||
| } | ||
| } | ||
| } | ||
| ); |
|
Thanks Tim! |
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Jul 13, 2021
Two parts here: 1. Don't synchonize on the async supplier. Even though these aren't actually async yet it seeems, the synchronization wouldn't work anyway if they were. If this were to be called concurrently the current approach would cause blocking needlessly though. Either way, synchonizing ever call and all the downstream calls is just unnecessarily hard on the JIT here. 2. The async look over the interceptors was needlessly slow with all the synchronization etc. in the `StepListener`. Also it made the stacktraces on transport thread extremely deep and complicated. Simplified and made this faster by using a normal listener/iterator without any synchronization.
original-brownbear
added a commit
that referenced
this pull request
Jul 13, 2021
Two parts here: 1. Don't synchonize on the async supplier. Even though these aren't actually async yet it seeems, the synchronization wouldn't work anyway if they were. If this were to be called concurrently the current approach would cause blocking needlessly though. Either way, synchonizing ever call and all the downstream calls is just unnecessarily hard on the JIT here. 2. The async look over the interceptors was needlessly slow with all the synchronization etc. in the `StepListener`. Also it made the stacktraces on transport thread extremely deep and complicated. Simplified and made this faster by using a normal listener/iterator without any synchronization.
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Jul 24, 2021
Same as elastic#75252 pretty much just continuing to make this logic a little simpler for easier profiling and (very) maybe performance through saving some allocations/indirection.
original-brownbear
added a commit
that referenced
this pull request
Jul 29, 2021
Same as #75252 pretty much just continuing to make this logic a little simpler for easier profiling and (very) maybe performance through saving some allocations/indirection.
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Jul 29, 2021
…5662) Same as elastic#75252 pretty much just continuing to make this logic a little simpler for easier profiling and (very) maybe performance through saving some allocations/indirection.
original-brownbear
added a commit
that referenced
this pull request
Jul 29, 2021
ywangd
pushed a commit
to ywangd/elasticsearch
that referenced
this pull request
Jul 30, 2021
…5662) Same as elastic#75252 pretty much just continuing to make this logic a little simpler for easier profiling and (very) maybe performance through saving some allocations/indirection.
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Apr 18, 2023
Same problem as in elastic#75252 addressed, also introduced some constants to reduce code size in hot methods.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Two parts here from analyzing auth slowness in recent SDHs:
actually async yet it seeems, the synchronization wouldn't work anyway
if they were. If this were to be called concurrently the current approach would cause
blocking needlessly though. Either way, synchonizing ever call and all the downstream calls
is just unnecessarily hard on the JIT here.
in the
StepListener. Also it made the stacktraces on transport thread extremely deep and complicated.Simplified and made this faster by using a normal listener/iterator without any synchronization.