Skip to content

Add configuration property "spring.task.execution.pool.shutdown.accept-tasks-after-context-close" #38968

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

Closed
wants to merge 1 commit into from

Conversation

quaff
Copy link
Contributor

@quaff quaff commented Jan 4, 2024

ExecutorConfigurationSupport::setAcceptTasksAfterContextClose is introduced since Spring Framework 6.1

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 4, 2024
@wilkinsona wilkinsona added this to the 3.3.x milestone Jan 4, 2024
@wilkinsona wilkinsona added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 4, 2024
@wilkinsona wilkinsona self-assigned this Jan 8, 2024
@wilkinsona
Copy link
Member

Thanks for the PR, @quaff.

We may need to find another location for this new property. The other two shutdown.* properties can be applied to both SimpleAsyncTaskExecutor and ThreadPoolTaskExecutor but this new one only applies to the latter. As such, it may be misleading when configured here. Perhaps we should introduce a pool-specific shutdown group and move it there.

Alternatively, I wonder if it would make sense for Framework to support this behavior with SimpleAsyncTaskExecutor? We'll discuss it with the Framework team.

@wilkinsona wilkinsona added the status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team label Jan 8, 2024
@wilkinsona wilkinsona removed their assignment Jan 8, 2024
@wilkinsona
Copy link
Member

From @jhoeller:

That property is indeed new for the thread pool variants based on java.util.concurrent. I see it as specifically purposed for thread pools and in particular the ScheduledExecutorService variants where the effect goes beyond not accepting user tasks anymore: It's primarily about cancelling all registered triggers and recurring tasks there, in preparation for a complete shutdown, which is the new default behavior for a graceful shutdown there now. The acceptTasksAfterContextClose flag is effectively an escape hatch for restoring the old behavior there, starting the shutdown procedure at the last moment even for triggers etc.

On SimpleAsyncTaskExecutor and also SimpleAsyncTaskScheduler, there never was such old behavior, so I don't see a need for such a flag there. SimpleAsyncTaskScheduler always starts the shutdown procedure of the scheduler thread when the context closes (equivalent to acceptTasksAfterContextClose=false but without an escape hatch). SimpleAsyncTaskExecutor does not really have a concept of an early shutdown signal since it does not have a shared thread - just the common enforcement of individual task termination if taskTerminationTimeout has been set.
All in all, from a Boot perspective, I'd consider acceptTasksAfterContextClose as an escape hatch that is specific to the thread pool variants.

It's also worth noting that the virtual thread based variants have a different notion of task rejection: The thread pool variants reject the acceptance of a task into their task queue (a backlog that may be quite populated already), whereas the virtual threads only do immediate execution and therefore only actually reject tasks on ultimate termination. That's another reason why the thread pool variants need to start rejecting early on context close for a graceful shutdown, in addition to the trigger/recurring task management.

Given the above, I think the configuration property should be spring.task.execution.pool.accept-tasks-after-context-close or spring.task.execution.pool.shutdown.accept-tasks-after-context-close. I'm leaning towards the latter, despite there being only one shutdown property at the moment. It provides some symmetry with the spring.task.execution.shutdown.* properties and also provides a natural home for any pool-specific shutdown-related properties that we may want to add in the future.

@wilkinsona wilkinsona added for: merge-with-amendments Needs some changes when we merge and removed status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team labels Jan 9, 2024
@quaff
Copy link
Contributor Author

quaff commented Jan 9, 2024

Given the above, I think the configuration property should be spring.task.execution.pool.accept-tasks-after-context-close or spring.task.execution.pool.shutdown.accept-tasks-after-context-close. I'm leaning towards the latter, despite there being only one shutdown property at the moment. It provides some symmetry with the spring.task.execution.shutdown.* properties and also provides a natural home for any pool-specific shutdown-related properties that we may want to add in the future.

OK, I will amend it.

…t-tasks-after-context-close"

ExecutorConfigurationSupport::setAcceptTasksAfterContextClose is introduced since Spring Framework 6.1
@quaff quaff changed the title Add configuration property "spring.task.execution.shutdown.accept-tasks-after-context-close" Add configuration property "spring.task.execution.pool.shutdown.accept-tasks-after-context-close" Jan 9, 2024
@quaff
Copy link
Contributor Author

quaff commented Jan 9, 2024

It's done, please review @wilkinsona

@mhalbritter mhalbritter removed the for: merge-with-amendments Needs some changes when we merge label Jan 10, 2024
@mhalbritter mhalbritter self-assigned this Jan 10, 2024
mhalbritter pushed a commit that referenced this pull request Jan 10, 2024
…t-tasks-after-context-close"

ExecutorConfigurationSupport::setAcceptTasksAfterContextClose is
introduced since Spring Framework 6.1

See gh-38968
mhalbritter added a commit that referenced this pull request Jan 10, 2024
@mhalbritter
Copy link
Contributor

Thank you very much!

@mhalbritter mhalbritter modified the milestones: 3.3.x, 3.3.0-M1 Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants