[
https://issues.apache.org/jira/browse/SLING-5416?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15093891#comment-15093891
]
Thomas Mueller commented on SLING-5416:
---------------------------------------
In my view, calling Thread.interrupt is a bug, because it causes problems.
> we don't ship a default configuration with Sling
I'm not asking to ship default configurations. I'm asking to change Sling so
that the default (if not explicitly configured differently) is graceful.
> we can't know beforehand what the consumers will use the thread pool for
We know it is used for Apache Oak indexing threads. We know consumers can call
Oak, and potentially call libraries that don't work well when calling
Thread.interrupt
> This also avoids any backwards compatibility issues.
Well, calling Thread.interrupt is a bug, and not changing that means Sling
stays buggy. Any code change is a potential backwards compatibility issue.
Potential backwards compatibility issues is not a reason to not fix bugs.
> 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)
