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

Clarify @EnableScheduling javadoc re ExecutorService shutdown [SPR-9280] #13918

Closed
spring-projects-issues opened this issue Mar 29, 2012 · 4 comments

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Mar 29, 2012

Grzegorz Grzybek opened SPR-9280 and commented

Javadoc for @EnableScheduling has the following code example:

 @Configuration
 @EnableScheduling
 public class AppConfig implements SchedulingConfigurer {
     @Override
     public void configureTasks(ScheduledTaskRegistrar taskRegistrar) {
         taskRegistrar.setScheduler(taskExecutor());
     }

     @Bean
     public Executor taskExecutor() {
         return Executors.newScheduledThreadPool(100);
     }
 }

With such @Bean definition (taskExecutor), the returned bean cannot be properly destroyed while closing the appcontext - returned TaskSheduler have "shutdown" method, but it cannot be invoked using "(inferred)" mechanism. So with e.g., web application it causes memory leaks.

Please either

  • change "taskExecutor" definition to return org.springframework.scheduling.concurrent.ThreadPoolTaskScheduler, or
  • use @Bean(destroyMethod = "shutdown")

one more thing - I have strange issues with:

@Bean(destroyMethod = "shutdown")
public Executor springTaskScheduler()
{
  return Executors.newScheduledThreadPool(10);
}

because although shutdown() is called, some classes from Spring (in heap dump) appear to have been on "native stack" (e.g., org.springframework.aop.framework.autoproxy.InfrastructureAdvisorAutoProxyCreator) - more on this on Spring Forum (soon)

regards
Grzegorz Grzybek


Affects: 3.1.1

Referenced from: commits 9a856c0, 1380d05

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 29, 2012

Grzegorz Grzybek commented

It's ok with destroyMethod = "shutdown" - first the webappclassloaders were not GCed, but after several minutes they were gone (maybe some finalizers were invoked).

Also - without using destroyMethod = "shutdown", the java.util.concurrent scheduled executor's shutdown is invoked by java.util.concurrent.ThreadPoolExecutor.finalize() - but as we're all taught - we should not rely on finalize() :)

So - maybe the issue should be closed.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Apr 13, 2012

Chris Beams commented

Hi Grzegorz,

Resolving this as invalid, as I'm not sure there's any remaining request to implement or change something here. Feel free to reopen or create a new issue if you're still seeing something missing.

Thanks,

Chris

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Apr 13, 2012

Grzegorz Grzybek commented

From my point of view it is only Javadoc that may be changed (clarified), because I had classloader leaks without proper destruction of returned Executor.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 15, 2012

Chris Beams commented

Javadoc updated, thanks Grzegorz!

commit 9a856c09f322b8ae821ff5e6a86dc0b0aeaf618d
Author: Chris Beams <cbeams@vmware.com>
Date:   Mon May 14 12:15:42 2012 +0300

    Clarify @EnableScheduling javadoc
    
    It is now advised that destroyMethod="shutdown" should be used
    on @Bean methods returning an ExecutorService.
    
    Issue: SPR-9280

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
1 participant