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

Platform-http do not spawn useless threads #1354

Merged
merged 2 commits into from
Jan 29, 2025

Conversation

Croway
Copy link
Contributor

@Croway Croway commented Jan 27, 2025

The previous implementation was creating 2 threads per request, the first one, created by spring framework via AsyncExecutionInterceptor, and the second one with CompletableFuture.runAsync.

This PR avoids the creation of the 2nd thread, this way, the behavior is like plain Spring Boot @Async. And the Apache Camel thread pool can be controlled via

spring.task.execution.pool.max-size=10
spring.task.execution.pool.core-size=10

While improving this behavior I faced an issue with the return object of the service method, in particular, as you can see from https://github.com/apache/camel-spring-boot/compare/main...Croway:suboptimal-thread-usage?expand=1#diff-7844a79d0aab67af46e8f0538438aa37f3c8b8afb04072724cc78fcf8acb17f5R84 the method still returns CompletableFuture, though I am not doing a runAsync or supplyAsync (the service method is already running asynchronously) and the return is CompletableFuture.completedFuture(null).

I tried using void instead of CompletableFuture, but I was getting the following error The request object has been recycled and is no longer associated with this facade when accessing the HttpServletRequest Object, I do not fully understand this behavior, maybe Spring expects a completable future.

@Croway Croway requested review from davsclaus and kulagaIA January 27, 2025 15:05
@Croway Croway marked this pull request as draft January 27, 2025 15:07
@farrault
Copy link

farrault commented Jan 27, 2025

I tried using void instead of CompletableFuture, but I was getting the following error The request object has been recycled and is no longer associated with this facade when accessing the HttpServletRequest Object, I do not fully understand this behavior, maybe Spring expects a completable future.

Hello @Croway
Spring MVC need to receive a CompletableFuture (or a DeferredResult ) in order to put the Http Request in async mode ( this specific behaviour is implemented in this class :
https://github.com/spring-projects/spring-framework/blob/main/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/DeferredResultMethodReturnValueHandler.java#L69 )

If not, my understanding is that the request is considered fully processed by the Web Server when the method returns and the request/response internal object are clear/reused by the WebServer (explaining the error message you got)

--

And as you may already have noticed, having a @Async annoted method returning a CompletableFuture.completedFuture(Object) is classic as stated in https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/scheduling/annotation/Async.html

--

That being said, may I ask why you preferred to keep the use of AsyncExecutionInterceptor instead of just keeping the CompletableFuture.runAsync here (and removing AsyncExecutionInterceptor) ?

Both seem fully equivalent to me in term of behaviour ( in particular : both returns a "real" CompletableFuture to Spring MVC representing a processing that is about to be executed inside the Spring Boot executor)
while the use of CompletableFuture.runAsync seems much simpler.

I find today that this was because of https://issues.apache.org/jira/browse/CAMEL-21318, that you introduced AsyncExecutionInterceptor in https://github.com/apache/camel-spring-boot/pull/1244/files
I need to admit, I don't understand what was your problem with Undertow ; but I don't see what characteristics AsyncExecutionInterceptor may have that CompletableFuture.runAsync(xxxx, executor) didn't have and would be needed to fix it. Did you spot exactly why it has fixed your problem ?

( Switching from the common ForkJoinPool to a Spring Boot thread executor in that PR was definitely useful though )

Regards
Fabien

@Croway Croway marked this pull request as ready for review January 29, 2025 14:00
@Croway Croway merged commit 671f91e into apache:main Jan 29, 2025
2 checks passed
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.

4 participants