Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move some security APIs to using promises in place of callbacks #123812

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

original-brownbear
Copy link
Member

@original-brownbear original-brownbear commented Mar 2, 2025

We ahve some incredibly deep callstacks in security that seem to visibly raise context switch costs, make profiling more complicated and generally make the code rather hard to follow.
Also, deeply nested listener chains tend to make logic consume more heap because we almost inevitably continue holding on to things we no longer need as we go down the stack.
Since the methods adjusted here mostly return a result synchronously on the hot path, we can both save overhead (we can do a couple follow-ups to that effect) and make things a little easier to follow by using promises as returns in place of consuming callbacks.

A good example of the effects of this change would be this piece of search request REST handling that flattens out enormously.

Before:
image

After:
image

We ahve some incredibly deep callstacks in security that seem to visibly
raise context switch costs, make profiling more complicated and
generally make the code rather hard to follow.
Since the methods adjusted here return a result synchronously we can
both save overhead and make things a little easier to follow by using
promises as returns in place of consuming callbacks.
@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Mar 2, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

Should we do something similar to flatten out RequestFilterChain#proceed rather than using recursion there too?

Comment on lines 657 to 661
try {
res.rawResult();
} catch (Exception e) {
listener.onFailure(e);
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather add something like this to SubscribableListener over exposing rawResult():

    public boolean completeIfFailed(ActionListener<?> listener) {
        final Object currentState = state;
        if (currentState instanceof FailureResult result) {
            listener.onFailure(result.exception());
            return true;
        } else {
            return false;
        }
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

I went for just an isSuccess now like other solutions have. There isn't much of a reason to be tricky for the failure path and this way we have a fast solution for the hot success path. Let me know what you think :) Combining this approach with the get-results on listenable futures would allow neat simplifications + speedups in some places I believe (by avoiding allocating new listeners).

@original-brownbear
Copy link
Member Author

Should we do something similar to flatten out RequestFilterChain#proceed rather than using recursion there too?

I think we should yea, gotta be a little careful maybe because of thread-context though. That kind of thing is made it impossible to go further with this kind of change in security for the time being when I tried because any thread-local context that only exists around the onResponse is lost if the listener hasn't been added before.

Comment on lines +272 to +273
public final boolean isSuccess() {
return state instanceof SuccessResult;
Copy link
Contributor

Choose a reason for hiding this comment

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

This bit LGTM, I'll leave the rest to the security team

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :Security/Security Security issues without another label Team:Security Meta label for security team v8.19.0 v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants