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

ThreadPoolTaskExecutor should cancel all remaining Future handles on shutdown [SPR-16607] #21148

Closed
spring-projects-issues opened this issue Mar 16, 2018 · 5 comments
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Mar 16, 2018

Nicolas Labrot opened SPR-16607 and commented

ThreadPoolTaskExecutor#submitListenable returns a future that will not be canceled when the executor is shutdown. By consequence Future#get will block indefinitely.

I would like to propose that the list of Runnable returned by this.executor.shutdownNow(); is used to cancel the ListenableFuture.


Affects: 5.0.4

Issue Links:

  • #21079 DefaultMessageListenerContainer should interrupt worker threads when not returning on shutdown

Referenced from: commits 3c1adf7, 9939908

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 18, 2018

Juergen Hoeller commented

This seems to apply not only to submitListenable but also to regular submit calls: All such tasks remain uncancelled if not executed yet and therefore returned from shutdownNow. It looks like we should generally cancel all such remaining Future tasks at that point.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 18, 2018

Nicolas Labrot commented

+1.

You may have note that the TaskDecorator may break the logic. A FutureTaskAwareTaskDecorator helper that decorate and still returns a FutureTask may be interesting.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 19, 2018

Juergen Hoeller commented

Indeed, we need to track decorated tasks in order to cancel the user-level Future handles if necessary. We're using a weak ConcurrentReferenceHashMap to track those while active: but only if actually necessary, i.e. if a TaskDecorator is configured and returns a different Runnable for a given task, avoiding any unnecessary storage of such task handles.

A similar problem arises in ThreadPoolTaskScheduler where user-level ListenableFuture handles are covered by a ScheduledFutureTask at runtime. We're tracking the user-level handles in the same way there, delivering consistent behavior for submitListenable next to plain submit calls (both of which are unusual on a ThreadPoolTaskScheduler in any case).

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 19, 2018

Nicolas Labrot commented

Thank you. I will be able to reuse it for a project using Spring 4.3.

In ExecutorConfigurationSupport#cancelRemainingTask is it normal to instanceof on RunnableFuture but in ThreadPoolTaskExecutor#cancelRemainingTask instanceof on Future ?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 19, 2018

Juergen Hoeller commented

Good point, even if ExecutorConfigurationSupport is always going to see a RunnableFuture with the current java.util.concurrent implementation (since the incoming parameter is a raw Runnable returned from shutdownNow), I've relaxed that check to just Future so that it looks consistent with the subclass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants