[
https://issues.apache.org/jira/browse/SLING-5416?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15093850#comment-15093850
]
Robert Munteanu commented on SLING-5416:
----------------------------------------
That's a good point. However, as we don't ship a default configuration with
Sling we can't know beforehand what the consumers will use the thread pool for.
There are perfectly valid use cases which work well with Thread.interrupt so my
suggestion is to keep the defaults unchanged and allow consumers to configure
the thread pool as they see fit. This also avoids any backwards compatibility
issues.
> Thread Pool should stop "gracefully"
> ------------------------------------
>
> Key: SLING-5416
> URL: https://issues.apache.org/jira/browse/SLING-5416
> Project: Sling
> Issue Type: Improvement
> Components: Commons
> Reporter: Thomas Mueller
>
> By default, the Sling thread pool manager calls "Thread.interrupt" to close
> the thread pool. This is because it is configured to be "not graceful" by
> default:
> {noformat}
> ./bundles/commons/threads/src/main/java/org/apache/sling/commons/threads/ModifiableThreadPoolConfig.java
> ...
> * The default values for this configuration are:
> ...
> * - shutdown graceful: false
> * - shutdown wait time: -1
> ...
>
> ./bundles/commons/threads/src/main/java/org/apache/sling/commons/threads/impl/DefaultThreadPool.java
> public void shutdown() {
> this.logger.info("Shutting down thread pool [{}] ...", name);
> if ( this.executor != null ) {
> if (this.configuration.isShutdownGraceful()) {
> this.executor.shutdown();
> } else {
> this.executor.shutdownNow();
> }
> try {
> if (this.configuration.getShutdownWaitTimeMs() > 0) {
> if
> (!this.executor.awaitTermination(this.configuration.getShutdownWaitTimeMs(),
> TimeUnit.MILLISECONDS)) {
> logger.warn("Running commands have not terminated
> within "
> + this.configuration.getShutdownWaitTimeMs()
> + "ms. Will shut them down by interruption");
> this.executor.shutdownNow(); // TODO: shouldn't this
> be outside the if statement?!
> }
> }
> } catch (final InterruptedException ie) {
> this.logger.error("Cannot shutdown thread pool [" + this.name
> + "]", ie);
> }
> {noformat}
> I think Sling should change the default to be graceful (not call
> Thread.interrupt()). Thread.interrupt can can result in many problems,
> because it closes channels, possibly TCP/IP connections, and so on. When
> using external libraries (JDBC, MongoDB, Apache Lucene,...) it is hard to
> ensure that Thread.interrupt does not cause problems.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
