Spring Security Consider replacing .flatMap with .handle where appropriate for performance reasons

Hello.

https://github.com/spring-projects/spring-security/blob/0c2a7e03f780113867dd23c7c6f5f407bc854aed/core/src/main/java/org/springframework/security/authentication/ReactiveAuthenticationManagerAdapter.java#L44-L51

cases like this can be replaced with Reactor's .handle operator to improve the performance.

Something like:

.handle( (t, sink) -> {
    try {
        sink.next(authenticationManager.authenticate(t));
    } catch(Throwable error) {
        sink.error(error);
    }
})

Or even:

if (a.isAuthenticated()) {
    sink.next(a);
} else {
    sink.complete();
}

Comment From: bsideup

Worth mentioning that the linked method can also be replaced with Mono.fromCallable(...).subscribeOn(Schedulers.elastic())

Comment From: rwinch

Thanks for the suggestion @bsideup!

Can you please explain why that is going to produce better performance? Also, how much performance increase are we talking about? I'm asking because I want to know if this is a micro optimization or something that will be really noticeable.

Thanks again!

Comment From: bsideup

@rwinch when you use .flatMap, you must create an inner publisher, subscribe on it (although there is a fast path in case of Callable publishers, but be aware that in case of Mono.error it will throw it)

Mono.fromCallable(...).subscribeOn(Schedulers.elastic()) here is ofc a clear winner, but, on a topic of .flatMap vs .handle, .handle wins with a lower number of allocations, better handling of the error case (does not throw internally), and it removes the overhead of operator assembly (e.g. hooks)

That's of course a micro optimization, but keeping it consistent improves the overall performance, especially in a critical path.

Comment From: spring-projects-issues

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

Comment From: jzheaux

Pardon our dust here as we do some issue cleanup. Feedback was already provided earlier, and I don't think the ticket has been fully addressed, yet, so let's keep the issue open.