Skip to content

FINERACT-2449: Replace unbounded SimpleAsyncTaskExecutor with ThreadPoolTaskExecutor#5432

Open
DeathGun44 wants to merge 1 commit intoapache:developfrom
DeathGun44:FINERACT-2449/replace-unbounded-executor
Open

FINERACT-2449: Replace unbounded SimpleAsyncTaskExecutor with ThreadPoolTaskExecutor#5432
DeathGun44 wants to merge 1 commit intoapache:developfrom
DeathGun44:FINERACT-2449/replace-unbounded-executor

Conversation

@DeathGun44
Copy link
Contributor

Description

This PR addresses FINERACT-2449.

Background

The current SimpleAsyncTaskExecutor creates a new thread for every task. While effective for light loads, this unbounded behavior poses a theoretical risk of thread exhaustion (OutOfMemoryError: unable to create native thread) under specific high-concurrency scenarios.

Motivation

Relying on an unbounded executor is contrary to Spring Boot best practices for production-grade financial systems. This change proactively addresses the risk before it manifests in production.

Solution

Component Change
SpringConfig.java Replace SimpleAsyncTaskExecutor with ThreadPoolTaskExecutor
FineractProperties.java Add eventTaskExecutorCorePoolSize and eventTaskExecutorMaxPoolSize
application.properties Add externalized config with CPU-aware defaults

Testing

SpringConfigTest.java (Added 8 unit tests proving bounded behavior)

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Write the commit message as per our guidelines
  • Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
  • Create/update unit or integration tests for verifying the changes made.
  • Follow our coding conventions.
  • Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
  • This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.

@vidakovic
Copy link
Contributor

I think there is a need to address this... but (having best practices in mind): let's first have a look here https://docs.spring.io/spring-boot/reference/features/task-execution-and-scheduling.html ... there are already (auto) configuration properties available, so why invent our own and create more Java configuration when they are not really necessary. I only cross-read that documentation quickly, but I think everything that is done here can be achieved with adding the proper "spring" prefixed properties to application.properties (with environment variable placeholders, sure); I think no extra/custom Java configuration would be needed.

@DeathGun44
Copy link
Contributor Author

Hi @vidakovic, thanks for the review.

I investigated using spring.task.execution.* as suggested. While those properties
correctly auto-configure a TaskExecutor for @Async and related infrastructure,
they don't fully address the requirement here for two reasons:

1. Synchronous by default
From the Spring Boot docs:

"The auto-configured AsyncTaskExecutor is used for: Execution of asynchronous tasks
using @EnableAsync, Spring MVC, Spring WebFlux, Spring WebSocket, JPA bootstrap..."

SimpleApplicationEventMulticaster is not in that list. Per the
Spring Framework JavaDoc:

"Default is equivalent to SyncTaskExecutor, executing all listeners synchronously
in the calling thread."

Unless setTaskExecutor() is explicitly called, event listeners run synchronously.

2. Security context propagation
Spring Boot's auto-configured executor is not wrapped with DelegatingSecurityContextAsyncTaskExecutor,
nor is it associated with the event multicaster—both of which are required for correct
async event handling in Fineract.

Because of this, a properties-only solution isn't sufficient. That said, I'm happy to
refactor the configuration to use Spring Boot's ThreadPoolTaskExecutorBuilder, so we
can still honor the standard spring.task.execution.* pool sizing properties while
explicitly wrapping the executor for security and wiring it into the multicaster.

Does that approach work for you?

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.

2 participants