Author: rhauch Date: 2009-12-17 15:38:21 -0500 (Thu, 17 Dec 2009) New Revision: 1451 Added: trunk/dna-common/src/main/java/org/jboss/dna/common/util/NamedThreadFactory.java Modified: trunk/dna-graph/src/main/java/org/jboss/dna/graph/connector/federation/FederatedRepositorySource.java trunk/dna-graph/src/main/java/org/jboss/dna/graph/search/SearchEngineIndexer.java trunk/dna-graph/src/test/java/org/jboss/dna/graph/connector/RepositorySourceLoadHarness.java trunk/dna-jcr/src/main/java/org/jboss/dna/jcr/JcrEngine.java trunk/dna-jcr/src/main/java/org/jboss/dna/jcr/JcrRepository.java trunk/dna-repository/src/main/java/org/jboss/dna/repository/DnaConfiguration.java trunk/dna-repository/src/main/java/org/jboss/dna/repository/DnaEngine.java trunk/dna-repository/src/main/java/org/jboss/dna/repository/sequencer/SequencingService.java Log: DNA-598 Shutting down JcrEngine returns, but leaves idle threads running (preventing shutdown of VM) for 60 seconds Fixed the problem with the shutdown leaving idle threads and blocking termination of the VM. After debugging (and using a new NameThreadFactory that creates threads with names beginning with a specified prefix), it's clear now that these extra threads were owned by the Executor used in the FederatedRepositorySource, and this executor was not being shutdown in the FederatedRepositorySource.close() method. The Executor was creating the threads with a timeout of 60sec, which makes sense. Fixing this now properly shuts down the Executor, and this fixes the problem. Added: trunk/dna-common/src/main/java/org/jboss/dna/common/util/NamedThreadFactory.java =================================================================== --- trunk/dna-common/src/main/java/org/jboss/dna/common/util/NamedThreadFactory.java (rev 0) +++ trunk/dna-common/src/main/java/org/jboss/dna/common/util/NamedThreadFactory.java 2009-12-17 20:38:21 UTC (rev 1451) @@ -0,0 +1,54 @@ +/* + * JBoss DNA (http://www.jboss.org/dna) + * See the COPYRIGHT.txt file distributed with this work for information + * regarding copyright ownership. Some portions may be licensed + * to Red Hat, Inc. under one or more contributor license agreements. + * See the AUTHORS.txt file in the distribution for a full listing of + * individual contributors. + * + * JBoss DNA is free software. Unless otherwise indicated, all code in JBoss DNA + * is licensed to you under the terms of the GNU Lesser General Public License as + * published by the Free Software Foundation; either version 2.1 of + * the License, or (at your option) any later version. + * + * JBoss DNA is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this software; if not, write to the Free + * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA + * 02110-1301 USA, or see the FSF site: http://www.fsf.org. + */ +package org.jboss.dna.common.util; + +import java.util.concurrent.Executors; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.atomic.AtomicInteger; + +/** + * The thread factory that lets us adjust the names of the threads. + * + * @see Executors#defaultThreadFactory() + */ +public class NamedThreadFactory implements ThreadFactory { + static final AtomicInteger poolNumber = new AtomicInteger(1); + final ThreadGroup group; + final AtomicInteger threadNumber = new AtomicInteger(1); + final String namePrefix; + + public NamedThreadFactory( String poolName ) { + SecurityManager s = System.getSecurityManager(); + group = (s != null) ? s.getThreadGroup() : Thread.currentThread().getThreadGroup(); + namePrefix = (poolName != null && poolName.trim().length() != 0 ? poolName : "pool") + "-" + poolNumber.getAndIncrement() + + "-thread-"; + } + + public Thread newThread( Runnable r ) { + Thread t = new Thread(group, r, namePrefix + threadNumber.getAndIncrement(), 0); + if (t.isDaemon()) t.setDaemon(false); + if (t.getPriority() != Thread.NORM_PRIORITY) t.setPriority(Thread.NORM_PRIORITY); + return t; + } +} Property changes on: trunk/dna-common/src/main/java/org/jboss/dna/common/util/NamedThreadFactory.java ___________________________________________________________________ Name: svn:keywords + Id Revision Name: svn:eol-style + LF Modified: trunk/dna-graph/src/main/java/org/jboss/dna/graph/connector/federation/FederatedRepositorySource.java =================================================================== --- trunk/dna-graph/src/main/java/org/jboss/dna/graph/connector/federation/FederatedRepositorySource.java 2009-12-16 21:06:41 UTC (rev 1450) +++ trunk/dna-graph/src/main/java/org/jboss/dna/graph/connector/federation/FederatedRepositorySource.java 2009-12-17 20:38:21 UTC (rev 1451) @@ -43,6 +43,7 @@ import org.jboss.dna.common.i18n.I18n; import org.jboss.dna.common.util.CheckArg; import org.jboss.dna.common.util.HashCode; +import org.jboss.dna.common.util.NamedThreadFactory; import org.jboss.dna.graph.DnaLexicon; import org.jboss.dna.graph.ExecutionContext; import org.jboss.dna.graph.GraphI18n; @@ -197,8 +198,13 @@ */ public void close() { synchronized (this) { - // Release the configuration ... - this.configuration = null; + if (this.configuration != null) { + // Release the configuration ... + if (this.configuration.getExecutor() != null) { + this.configuration.getExecutor().shutdown(); + } + this.configuration = null; + } } } @@ -390,7 +396,7 @@ } // Create the ExecutorService ... - ExecutorService executor = Executors.newCachedThreadPool(); + ExecutorService executor = Executors.newCachedThreadPool(new NamedThreadFactory(name)); return new FederatedRepository(name, connectionFactory, workspaces, defaultCachePolicy, executor); } catch (RepositorySourceException t) { @@ -444,7 +450,7 @@ } } else { connectionFactory = context.getRepositoryConnectionFactory(); - executor = Executors.newCachedThreadPool(); + executor = Executors.newCachedThreadPool(new NamedThreadFactory(name)); } // Add the new workspace ... Modified: trunk/dna-graph/src/main/java/org/jboss/dna/graph/search/SearchEngineIndexer.java =================================================================== --- trunk/dna-graph/src/main/java/org/jboss/dna/graph/search/SearchEngineIndexer.java 2009-12-16 21:06:41 UTC (rev 1450) +++ trunk/dna-graph/src/main/java/org/jboss/dna/graph/search/SearchEngineIndexer.java 2009-12-17 20:38:21 UTC (rev 1451) @@ -34,6 +34,7 @@ import org.jboss.dna.common.i18n.I18n; import org.jboss.dna.common.util.CheckArg; import org.jboss.dna.common.util.Logger; +import org.jboss.dna.common.util.NamedThreadFactory; import org.jboss.dna.graph.ExecutionContext; import org.jboss.dna.graph.GraphI18n; import org.jboss.dna.graph.Location; @@ -71,7 +72,7 @@ private final String sourceName; private final SearchEngine searchEngine; private final int maxDepthPerRead = DEFAULT_MAX_DEPTH_PER_READ; - private final ExecutorService service = Executors.newSingleThreadExecutor(); + private final ExecutorService service; private final CompositeRequestChannel channel; private final SearchEngineProcessor processor; private boolean closed = false; @@ -91,8 +92,8 @@ * @throws IllegalArgumentException if the search engine or connection factory references are null */ public SearchEngineIndexer( ExecutionContext context, - SearchEngine searchEngine, - RepositoryConnectionFactory connectionFactory ) { + SearchEngine searchEngine, + RepositoryConnectionFactory connectionFactory ) { CheckArg.isNotNull(context, "context"); CheckArg.isNotNull(searchEngine, "searchEngine"); CheckArg.isNotNull(connectionFactory, "connectionFactory"); @@ -101,6 +102,7 @@ this.sourceName = searchEngine.getSourceName(); this.connectionFactory = connectionFactory; this.channel = new CompositeRequestChannel(this.sourceName); + this.service = Executors.newSingleThreadExecutor(new NamedThreadFactory("search-" + sourceName)); // Start the channel and search engine processor right away (this is why this object must be closed) this.channel.start(service, this.context, this.connectionFactory); this.processor = this.searchEngine.createProcessor(this.context, null, false); @@ -172,7 +174,7 @@ * @throws InvalidWorkspaceException if there is no workspace with the supplied name */ public SearchEngineIndexer index( String workspaceName, - Path path ) { + Path path ) { checkNotClosed(); CheckArg.isNotNull(workspaceName, "workspaceName"); CheckArg.isNotNull(path, "path"); @@ -192,8 +194,8 @@ * @throws InvalidWorkspaceException if there is no workspace with the supplied name */ public SearchEngineIndexer index( String workspaceName, - Path path, - int depth ) { + Path path, + int depth ) { checkNotClosed(); CheckArg.isNotNull(workspaceName, "workspaceName"); CheckArg.isNotNull(path, "path"); @@ -216,7 +218,7 @@ * @throws InvalidWorkspaceException if there is no workspace with the supplied name */ public SearchEngineIndexer index( String workspaceName, - Location location ) { + Location location ) { checkNotClosed(); CheckArg.isNotNull(workspaceName, "workspaceName"); CheckArg.isNotNull(location, "location"); @@ -236,8 +238,8 @@ * @throws InvalidWorkspaceException if there is no workspace with the supplied name */ public SearchEngineIndexer index( String workspaceName, - Location location, - int depth ) { + Location location, + int depth ) { checkNotClosed(); CheckArg.isNotNull(workspaceName, "workspaceName"); CheckArg.isNotNull(location, "location"); Modified: trunk/dna-graph/src/test/java/org/jboss/dna/graph/connector/RepositorySourceLoadHarness.java =================================================================== --- trunk/dna-graph/src/test/java/org/jboss/dna/graph/connector/RepositorySourceLoadHarness.java 2009-12-16 21:06:41 UTC (rev 1450) +++ trunk/dna-graph/src/test/java/org/jboss/dna/graph/connector/RepositorySourceLoadHarness.java 2009-12-17 20:38:21 UTC (rev 1451) @@ -35,6 +35,7 @@ import java.util.concurrent.TimeUnit; import org.jboss.dna.common.i18n.MockI18n; import org.jboss.dna.common.util.Logger; +import org.jboss.dna.common.util.NamedThreadFactory; import org.jboss.dna.graph.ExecutionContext; import org.jboss.dna.graph.Location; import org.jboss.dna.graph.property.basic.RootPath; @@ -100,7 +101,8 @@ // Create an Executor Service, using a thread factory that makes the first 'n' thread all wait for each other ... ExecutorService clientPool = null; if (clients.size() == 1) { - clientPool = Executors.newSingleThreadExecutor(); + ThreadFactory threadFactory = new NamedThreadFactory("load"); + clientPool = Executors.newSingleThreadExecutor(threadFactory); } else { final ThreadFactory threadFactory = new TestThreadFactory(clients.size()); clientPool = Executors.newFixedThreadPool(clients.size(), threadFactory); Modified: trunk/dna-jcr/src/main/java/org/jboss/dna/jcr/JcrEngine.java =================================================================== --- trunk/dna-jcr/src/main/java/org/jboss/dna/jcr/JcrEngine.java 2009-12-16 21:06:41 UTC (rev 1450) +++ trunk/dna-jcr/src/main/java/org/jboss/dna/jcr/JcrEngine.java 2009-12-17 20:38:21 UTC (rev 1451) @@ -114,8 +114,18 @@ @Override public void shutdown() { scheduler.shutdown(); + super.shutdown(); - super.shutdown(); + try { + this.repositoriesLock.lock(); + // Shut down all of the repositories ... + for (JcrRepository repository : repositories.values()) { + repository.close(); + } + this.repositories.clear(); + } finally { + this.repositoriesLock.unlock(); + } } @Override Modified: trunk/dna-jcr/src/main/java/org/jboss/dna/jcr/JcrRepository.java =================================================================== --- trunk/dna-jcr/src/main/java/org/jboss/dna/jcr/JcrRepository.java 2009-12-16 21:06:41 UTC (rev 1450) +++ trunk/dna-jcr/src/main/java/org/jboss/dna/jcr/JcrRepository.java 2009-12-17 20:38:21 UTC (rev 1451) @@ -963,6 +963,12 @@ return session; } + void close() { + if (this.federatedSource != null) { + this.federatedSource.close(); + } + } + /** * Returns the lock manager for the named workspace (if one already exists) or creates a new lock manager and returns it. This * method is thread-safe. Modified: trunk/dna-repository/src/main/java/org/jboss/dna/repository/DnaConfiguration.java =================================================================== --- trunk/dna-repository/src/main/java/org/jboss/dna/repository/DnaConfiguration.java 2009-12-16 21:06:41 UTC (rev 1450) +++ trunk/dna-repository/src/main/java/org/jboss/dna/repository/DnaConfiguration.java 2009-12-17 20:38:21 UTC (rev 1451) @@ -114,10 +114,22 @@ source.setDefaultWorkspaceName(DEFAULT_WORKSPACE_NAME); // The file was imported successfully, so now create the content information ... - configurationContent = new ConfigurationDefinition(source, null, null, context, null); + configurationContent = new ConfigurationDefinition("dna", source, null, null, context, null); } /** + * Set the name of this configuration. + * + * @param name the configuration name; may be null if the default name should be used + * @return this configuration + */ + public DnaConfiguration withName( String name ) { + if (name != null) name = "dna"; + configurationContent = configurationContent.with(name); + return this; + } + + /** * Load the configuration from a file at the given path. * * @param pathToConfigurationFile the path the file containing the configuration information @@ -260,7 +272,8 @@ graph.importXmlFrom(configurationFileInputStream).skippingRootElement(true).into(pathToParent); // The file was imported successfully, so now create the content information ... - configurationContent = new ConfigurationDefinition(source, null, pathToParent, context, null); + configurationContent = new ConfigurationDefinition(configurationContent.getName(), source, null, pathToParent, context, + null); return this; } @@ -332,7 +345,8 @@ assert parent != null; // Now create the content information ... - configurationContent = new ConfigurationDefinition(source, workspaceName, path, context, null); + configurationContent = new ConfigurationDefinition(configurationContent.getName(), source, workspaceName, path, context, + null); return this; } @@ -1301,6 +1315,7 @@ */ @Immutable public static class ConfigurationDefinition { + private final String name; private final ClassLoaderFactory classLoaderFactory; private final RepositorySource source; private final Path path; @@ -1308,11 +1323,14 @@ private final ExecutionContext context; private Graph graph; - protected ConfigurationDefinition( RepositorySource source, + protected ConfigurationDefinition( String configurationName, + RepositorySource source, String workspace, Path path, ExecutionContext context, ClassLoaderFactory classLoaderFactory ) { + assert configurationName != null; + this.name = configurationName; this.source = source; this.path = path != null ? path : RootPath.INSTANCE; this.workspace = workspace; @@ -1321,6 +1339,15 @@ } /** + * Get the name of this configuration. + * + * @return the configuration's name; never null + */ + public String getName() { + return name; + } + + /** * Get the repository source where the configuration content may be found * * @return the source for the configuration repository; never null @@ -1362,13 +1389,24 @@ } /** + * Return a copy of this configuration that uses the supplied name instead of this object's {@link #getPath() path}. + * + * @param name the desired name for the new configuration; if null, then the name of this configuration is used + * @return the new configuration + */ + public ConfigurationDefinition with( String name ) { + if (name == null) name = this.name; + return new ConfigurationDefinition(name, source, workspace, path, context, classLoaderFactory); + } + + /** * Return a copy of this configuration that uses the supplied path instead of this object's {@link #getPath() path}. * * @param path the desired path for the new configuration; if null, then "/" is used * @return the new configuration */ public ConfigurationDefinition with( Path path ) { - return new ConfigurationDefinition(source, workspace, path, context, classLoaderFactory); + return new ConfigurationDefinition(name, source, workspace, path, context, classLoaderFactory); } /** @@ -1379,7 +1417,7 @@ * @return the new configuration */ public ConfigurationDefinition withWorkspace( String workspace ) { - return new ConfigurationDefinition(source, workspace, path, context, classLoaderFactory); + return new ConfigurationDefinition(name, source, workspace, path, context, classLoaderFactory); } /** @@ -1390,7 +1428,7 @@ * @return the new configuration */ public ConfigurationDefinition with( ClassLoaderFactory classLoaderFactory ) { - return new ConfigurationDefinition(source, workspace, path, context, classLoaderFactory); + return new ConfigurationDefinition(name, source, workspace, path, context, classLoaderFactory); } /** Modified: trunk/dna-repository/src/main/java/org/jboss/dna/repository/DnaEngine.java =================================================================== --- trunk/dna-repository/src/main/java/org/jboss/dna/repository/DnaEngine.java 2009-12-16 21:06:41 UTC (rev 1450) +++ trunk/dna-repository/src/main/java/org/jboss/dna/repository/DnaEngine.java 2009-12-17 20:38:21 UTC (rev 1451) @@ -28,7 +28,8 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.ExecutorService; -import java.util.concurrent.ScheduledThreadPoolExecutor; +import java.util.concurrent.Executors; +import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeUnit; import net.jcip.annotations.Immutable; import org.jboss.dna.common.collection.Problem; @@ -36,6 +37,7 @@ import org.jboss.dna.common.collection.SimpleProblems; import org.jboss.dna.common.util.CheckArg; import org.jboss.dna.common.util.Logger; +import org.jboss.dna.common.util.NamedThreadFactory; import org.jboss.dna.graph.ExecutionContext; import org.jboss.dna.graph.Graph; import org.jboss.dna.graph.JcrLexicon; @@ -119,7 +121,8 @@ configurationChangeBus.register(repositoryService); // Create the sequencing service ... - executorService = new ScheduledThreadPoolExecutor(10); // Use a magic number for now + ThreadFactory threadPoolFactory = new NamedThreadFactory(configuration.getName()); + executorService = Executors.newScheduledThreadPool(10, threadPoolFactory); sequencingService = new SequencingService(); sequencingService.setExecutionContext(context); sequencingService.setExecutorService(executorService); @@ -292,18 +295,13 @@ * @see #start() */ public void shutdown() { + // Then terminate the executor service, which may be running background jobs that are not yet completed + // and which will prevent new jobs being submitted (to the sequencing service) ... + executorService.shutdown(); + // First, shutdown the sequencing service, which will prevent any additional jobs from going through ... sequencingService.getAdministrator().shutdown(); - // Then terminate the executor service, which may be running background jobs that are not yet completed ... - try { - executorService.awaitTermination(10 * 60, TimeUnit.SECONDS); // No TimeUnit.MINUTES in JDK 5 - } catch (InterruptedException ie) { - // Reset the thread's status and continue this method ... - Thread.interrupted(); - } - executorService.shutdown(); - // Finally shut down the repository source, which closes all connections ... repositoryService.getAdministrator().shutdown(); } Modified: trunk/dna-repository/src/main/java/org/jboss/dna/repository/sequencer/SequencingService.java =================================================================== --- trunk/dna-repository/src/main/java/org/jboss/dna/repository/sequencer/SequencingService.java 2009-12-16 21:06:41 UTC (rev 1450) +++ trunk/dna-repository/src/main/java/org/jboss/dna/repository/sequencer/SequencingService.java 2009-12-17 20:38:21 UTC (rev 1451) @@ -43,6 +43,7 @@ import org.jboss.dna.common.util.CheckArg; import org.jboss.dna.common.util.HashCode; import org.jboss.dna.common.util.Logger; +import org.jboss.dna.common.util.NamedThreadFactory; import org.jboss.dna.graph.ExecutionContext; import org.jboss.dna.graph.Graph; import org.jboss.dna.graph.Node; @@ -327,7 +328,7 @@ * @return the executor service */ protected ExecutorService createDefaultExecutorService() { - return Executors.newSingleThreadExecutor(); + return Executors.newSingleThreadExecutor(new NamedThreadFactory("sequencing")); } protected void startService() {