Hi,
so as you I've been working on allowing web applications to properly
undeploy without leaking permanent generation memory. I have a first
cut that solves all the problems I've been able to find so far.
Unfortunately it still does not allow GS to actually undeploy cleanly,
but it's a step in the right direction and makes debugging whatever
else is missing easier.
The set of issues I've been looking into is detailed in this
page:
http://wiki.apache.org/tomcat/MemoryLeakProtection
Basically our referencing/coverage subsystems violated every rule in
that page, and I guess more, since GS still does not deploys cleanly
and I'm at a loss at finding more reasons for that ;-)
Anyways, the attached patch contains changes that either handle
the issue or make it easier to look for further problems.
In particular:
- cleans up all the thread locals I could actually get rid of
- it exposes a CRS.cleaupThreadLocals() for the two thread locals
that I could not get rid of and that cannot be easily removed
either. A web application must add a filter that calls that
method after each request (annoying, I know)
- adds methods to empty all SPI registries before shutdown
- changes CRS2GeoTiffMetadataAdapter to use the factories exposed
by the referencing subsystem instead of creating its own
transform factory (which in turn holds another SPI registry).
Don't worry for this one, the transform factory returned
by the finder is cached itself, so it's really just removing
a duplication
- removes the ThreadedEPSGFactory shutdown hook, a thread
that is registered globally in the JVM for the purpose of
shutting down the EPSG database connections... something
that is completely not needed, as the connections simple cease
existing and the embedded databases are all working read only
(so they can be killed abruptly without issues)
As for adding a method in GeoTools that makes all the necessary
calls to shut down the application, it can get tricky due
to dependencies (it should be in a central module, but from
that central module you don't see all the necessary dependencies).
So what about a page explaining what one should do instead?
Is there a place in the sphinx docs for that?
I plan to commit the changes tomorrow on trunk only
unless someone objects
Cheers
Andrea
--
Andrea Aime
OpenGeo - http://opengeo.org
Expert service straight from the developers.
diff --git a/modules/library/coverage/src/main/java/org/geotools/coverage/grid/io/imageio/geotiff/GeoTiffMetadata2CRSAdapter.java b/modules/library/coverage/src/main/java/org/geotools/coverage/grid/io/imageio/geotiff/GeoTiffMetadata2CRSAdapter.java
index 11abf7f..0ca06ef 100644
--- a/modules/library/coverage/src/main/java/org/geotools/coverage/grid/io/imageio/geotiff/GeoTiffMetadata2CRSAdapter.java
+++ b/modules/library/coverage/src/main/java/org/geotools/coverage/grid/io/imageio/geotiff/GeoTiffMetadata2CRSAdapter.java
@@ -50,6 +50,7 @@ import javax.measure.unit.Unit;
import org.geotools.coverage.grid.io.imageio.geotiff.codes.GeoTiffCoordinateTransformationsCodes;
import org.geotools.coverage.grid.io.imageio.geotiff.codes.GeoTiffGCSCodes;
import org.geotools.coverage.grid.io.imageio.geotiff.codes.GeoTiffPCSCodes;
+import org.geotools.factory.CommonFactoryFinder;
import org.geotools.factory.Hints;
import org.geotools.metadata.iso.citation.CitationImpl;
import org.geotools.referencing.CRS;
@@ -154,7 +155,7 @@ public final class GeoTiffMetadata2CRSAdapter {
* Cached {...@link MathTransformFactory} for building {...@link MathTransform}
* objects.
*/
- private final static MathTransformFactory mtFactory = new DefaultMathTransformFactory();
+ private MathTransformFactory mtFactory = ReferencingFactoryFinder.getMathTransformFactory(null);
/**
* The default value for {...@link #maxStrongReferences} .
@@ -1799,4 +1800,5 @@ public final class GeoTiffMetadata2CRSAdapter {
public Hints getHints() {
return hints;
}
+
}
diff --git a/modules/library/main/src/main/java/org/geotools/data/DataAccessFinder.java b/modules/library/main/src/main/java/org/geotools/data/DataAccessFinder.java
index 59101c1..41fd626 100644
--- a/modules/library/main/src/main/java/org/geotools/data/DataAccessFinder.java
+++ b/modules/library/main/src/main/java/org/geotools/data/DataAccessFinder.java
@@ -242,4 +242,15 @@ public final class DataAccessFinder {
DataStoreFinder.scanForPlugins();
getServiceRegistry().scanForPlugins();
}
+
+ /**
+ * Resets the factory finder and prepares for a new full scan of the SPI subsystems
+ */
+ public static void reset() {
+ FactoryRegistry copy = registry;
+ registry = null;
+ if(copy != null) {
+ copy.deregisterAll();
+ }
+ }
}
diff --git a/modules/library/main/src/main/java/org/geotools/data/DataStoreFinder.java b/modules/library/main/src/main/java/org/geotools/data/DataStoreFinder.java
index 3a6a0fa..b2c254c 100644
--- a/modules/library/main/src/main/java/org/geotools/data/DataStoreFinder.java
+++ b/modules/library/main/src/main/java/org/geotools/data/DataStoreFinder.java
@@ -143,5 +143,16 @@ public final class DataStoreFinder {
getServiceRegistry().scanForPlugins();
}
+
+ /**
+ * Resets the factory finder and prepares for a new full scan of the SPI subsystems
+ */
+ public static void reset() {
+ FactoryRegistry copy = registry;
+ registry = null;
+ if(copy != null) {
+ copy.deregisterAll();
+ }
+ }
}
diff --git a/modules/library/main/src/main/java/org/geotools/factory/CommonFactoryFinder.java b/modules/library/main/src/main/java/org/geotools/factory/CommonFactoryFinder.java
index 62f0517..7b2e4d8 100644
--- a/modules/library/main/src/main/java/org/geotools/factory/CommonFactoryFinder.java
+++ b/modules/library/main/src/main/java/org/geotools/factory/CommonFactoryFinder.java
@@ -326,4 +326,15 @@ public final class CommonFactoryFinder extends FactoryFinder {
registry.scanForPlugins();
}
}
+
+ /**
+ * Resets the factory finder and prepares for a new full scan of the SPI subsystems
+ */
+ public static void reset() {
+ FactoryRegistry copy = registry;
+ registry = null;
+ if(copy != null) {
+ copy.deregisterAll();
+ }
+ }
}
diff --git a/modules/library/referencing/src/main/java/org/geotools/referencing/CRS.java b/modules/library/referencing/src/main/java/org/geotools/referencing/CRS.java
index 165f43d..9ee0573 100644
--- a/modules/library/referencing/src/main/java/org/geotools/referencing/CRS.java
+++ b/modules/library/referencing/src/main/java/org/geotools/referencing/CRS.java
@@ -64,8 +64,10 @@ import org.geotools.referencing.cs.DefaultEllipsoidalCS;
import org.geotools.referencing.cs.DefaultCoordinateSystemAxis;
import org.geotools.referencing.factory.AbstractAuthorityFactory;
import org.geotools.referencing.factory.IdentifiedObjectFinder;
+import org.geotools.referencing.operation.DefaultMathTransformFactory;
import org.geotools.referencing.operation.projection.MapProjection;
import org.geotools.referencing.operation.transform.IdentityTransform;
+import org.geotools.referencing.wkt.Formattable;
import org.geotools.resources.geometry.XRectangle2D;
import org.geotools.resources.CRSUtilities;
import org.geotools.resources.i18n.Errors;
@@ -1679,6 +1681,16 @@ search: if (DefaultCoordinateSystemAxis.isCompassDirection(axis.getD
strictFactory = null;
lenientFactory = null;
}
+
+ /**
+ * Cleans up the thread local set in this thread. They can prevent web applications from
+ * proper shutdown
+ */
+ public static void cleanupThreadLocals() {
+ DefaultMathTransformFactory.cleanupThreadLocals();
+ Formattable.cleanupThreadLocals();
+ }
+
/**
* Prints to the {...@linkplain System#out standard output stream} some information about
diff --git a/modules/library/referencing/src/main/java/org/geotools/referencing/ReferencingFactoryFinder.java b/modules/library/referencing/src/main/java/org/geotools/referencing/ReferencingFactoryFinder.java
index fc3b34c..92a8330 100644
--- a/modules/library/referencing/src/main/java/org/geotools/referencing/ReferencingFactoryFinder.java
+++ b/modules/library/referencing/src/main/java/org/geotools/referencing/ReferencingFactoryFinder.java
@@ -709,7 +709,11 @@ loop: for (int i=0; ; i++) {
* Resets the factory finder and prepares for a new full scan of the SPI subsystems
*/
public static void reset() {
+ FactoryRegistry copy = registry;
registry = null;
+ if(copy != null) {
+ copy.deregisterAll();
+ }
}
/**
diff --git a/modules/library/referencing/src/main/java/org/geotools/referencing/crs/AbstractDerivedCRS.java b/modules/library/referencing/src/main/java/org/geotools/referencing/crs/AbstractDerivedCRS.java
index 0b2ac82..79d4fb9 100644
--- a/modules/library/referencing/src/main/java/org/geotools/referencing/crs/AbstractDerivedCRS.java
+++ b/modules/library/referencing/src/main/java/org/geotools/referencing/crs/AbstractDerivedCRS.java
@@ -341,7 +341,7 @@ public class AbstractDerivedCRS extends AbstractSingleCRS implements GeneralDeri
that.conversionFromBase,
compareMetadata);
} finally {
- _COMPARING.set(Boolean.FALSE);
+ _COMPARING.remove();
}
}
}
diff --git a/modules/library/referencing/src/main/java/org/geotools/referencing/factory/AuthorityFactoryAdapter.java b/modules/library/referencing/src/main/java/org/geotools/referencing/factory/AuthorityFactoryAdapter.java
index f333d91..81c705a 100644
--- a/modules/library/referencing/src/main/java/org/geotools/referencing/factory/AuthorityFactoryAdapter.java
+++ b/modules/library/referencing/src/main/java/org/geotools/referencing/factory/AuthorityFactoryAdapter.java
@@ -1263,4 +1263,19 @@ public class AuthorityFactoryAdapter extends AbstractAuthorityFactory implements
}
return false;
}
+
+ @Override
+ public void dispose() throws FactoryException {
+ super.dispose();
+ disposeAbstractAuthorityFactory(datumFactory);
+ disposeAbstractAuthorityFactory(csFactory);
+ disposeAbstractAuthorityFactory(crsFactory);
+ disposeAbstractAuthorityFactory(operationFactory);
+ }
+
+ private void disposeAbstractAuthorityFactory(Object factory) throws FactoryException {
+ if(factory instanceof AbstractAuthorityFactory) {
+ ((AbstractAuthorityFactory) factory).dispose();
+ }
+ }
}
diff --git a/modules/library/referencing/src/main/java/org/geotools/referencing/factory/DeferredAuthorityFactory.java b/modules/library/referencing/src/main/java/org/geotools/referencing/factory/DeferredAuthorityFactory.java
index 5cf129d..b895884 100644
--- a/modules/library/referencing/src/main/java/org/geotools/referencing/factory/DeferredAuthorityFactory.java
+++ b/modules/library/referencing/src/main/java/org/geotools/referencing/factory/DeferredAuthorityFactory.java
@@ -52,10 +52,8 @@ public abstract class DeferredAuthorityFactory extends BufferedAuthorityFactory
{
/**
* The timer for {...@linkplain AbstractAuthorityFactory#dispose disposing} backing stores.
- *
- * @todo Give a name to this timer when we will be allowed to compile for J2SE 1.5.
*/
- private static final Timer TIMER = new Timer(true);
+ private static Timer TIMER = new Timer("GT authority factory disposer", true);
/**
* The task for disposing the backing store, or {...@code null} if none.
@@ -170,6 +168,10 @@ public abstract class DeferredAuthorityFactory extends BufferedAuthorityFactory
* much as twice that time before to be closed.
*/
public synchronized void setTimeout(final long delay) {
+ // if we have been disposed, don't do anything
+ if(TIMER == null)
+ return;
+
if (disposer != null) {
disposer.cancel();
}
@@ -202,6 +204,16 @@ public abstract class DeferredAuthorityFactory extends BufferedAuthorityFactory
}
super.dispose();
}
+
+ /**
+ * Gets rid of the timer thread at application shutdown
+ */
+ public static void exit() {
+ if(TIMER != null) {
+ TIMER.cancel();
+ TIMER = null;
+ }
+ }
/**
* The task for closing the backing store after the timeout.
diff --git a/modules/library/referencing/src/main/java/org/geotools/referencing/factory/ManyAuthoritiesFactory.java b/modules/library/referencing/src/main/java/org/geotools/referencing/factory/ManyAuthoritiesFactory.java
index 8c11327..7a63eda 100644
--- a/modules/library/referencing/src/main/java/org/geotools/referencing/factory/ManyAuthoritiesFactory.java
+++ b/modules/library/referencing/src/main/java/org/geotools/referencing/factory/ManyAuthoritiesFactory.java
@@ -585,8 +585,8 @@ public class ManyAuthoritiesFactory extends AuthorityFactoryAdapter implements C
final Set<String> codes = new LinkedHashSet<String>();
final Set<AuthorityFactory> done = new HashSet<AuthorityFactory>();
done.add(this); // Safety for avoiding recursive calls.
- inProgress.set(Boolean.TRUE);
try {
+ inProgress.set(Boolean.TRUE);
for (String authority : getAuthorityNames()) {
authority = authority.trim();
final char separator = getSeparator(authority);
diff --git a/modules/library/referencing/src/main/java/org/geotools/referencing/factory/epsg/DirectEpsgFactory.java b/modules/library/referencing/src/main/java/org/geotools/referencing/factory/epsg/DirectEpsgFactory.java
index b0ab4ac..6ad5a08 100644
--- a/modules/library/referencing/src/main/java/org/geotools/referencing/factory/epsg/DirectEpsgFactory.java
+++ b/modules/library/referencing/src/main/java/org/geotools/referencing/factory/epsg/DirectEpsgFactory.java
@@ -2988,7 +2988,6 @@ public abstract class DirectEpsgFactory extends DirectAuthorityFactory
*/
@Override
public synchronized void dispose() throws FactoryException {
- final boolean shutdown = SHUTDOWN_THREAD.equals(Thread.currentThread().getName());
final boolean isClosed;
try {
Connection connection = getConnection();
@@ -3006,16 +3005,14 @@ public abstract class DirectEpsgFactory extends DirectAuthorityFactory
((PreparedStatement) it.next()).close();
it.remove();
}
- if (shutdown) {
- shutdown(true);
- }
+ shutdown(true);
connection.close();
dataSource = null;
} catch (SQLException exception) {
throw new FactoryException(exception);
}
super.dispose();
- if (shutdown) try {
+ try {
shutdown(false);
} catch (SQLException exception) {
throw new FactoryException(exception);
diff --git a/modules/library/referencing/src/main/java/org/geotools/referencing/factory/epsg/ThreadedEpsgFactory.java b/modules/library/referencing/src/main/java/org/geotools/referencing/factory/epsg/ThreadedEpsgFactory.java
index bc71efe..71c7523 100644
--- a/modules/library/referencing/src/main/java/org/geotools/referencing/factory/epsg/ThreadedEpsgFactory.java
+++ b/modules/library/referencing/src/main/java/org/geotools/referencing/factory/epsg/ThreadedEpsgFactory.java
@@ -137,11 +137,6 @@ public class ThreadedEpsgFactory extends DeferredAuthorityFactory
private DataSource datasource;
/**
- * The shutdown hook, or {...@code null} if none.
- */
- private Thread shutdown;
-
- /**
* Constructs an authority factory using the default set of factories.
*/
public ThreadedEpsgFactory() {
@@ -457,57 +452,4 @@ public class ThreadedEpsgFactory extends DeferredAuthorityFactory
return super.canDisposeBackingStore(backingStore);
}
- /**
- * Ensures that the database connection will be closed on JVM exit. This code will be executed
- * even if the JVM is terminated because of an exception or with [Ctrl-C]. Note: we create this
- * shutdown hook only if this factory is registered as a service because it will prevent this
- * instance to be garbage collected until it is deregistered.
- */
- private final class ShutdownHook extends Thread {
- public ShutdownHook() {
- super(DirectEpsgFactory.SHUTDOWN_THREAD);
- }
-
- @Override
- public void run() {
- synchronized (ThreadedEpsgFactory.this) {
- try {
- dispose();
- } catch (Throwable exception) {
- // Too late for logging, since the JVM is
- // in process of shutting down. Ignore...
- }
- }
- }
- }
-
- /**
- * Called when this factory is added to the given {...@code category} of the given
- * {...@code registry}. The object may already be registered under another category.
- */
- @Override
- public synchronized void onRegistration(final ServiceRegistry registry, final Class category) {
- super.onRegistration(registry, category);
- if (shutdown == null) {
- shutdown = new ShutdownHook();
- Runtime.getRuntime().addShutdownHook(shutdown);
- }
- }
-
- /**
- * Called when this factory is removed from the given {...@code category} of the given
- * {...@code registry}. The object may still be registered under another category.
- */
- @Override
- public synchronized void onDeregistration(final ServiceRegistry registry, final Class category) {
- if (shutdown != null) {
- if (registry.getServiceProviderByClass(getClass()) == null) {
- // Remove the shutdown hook only if this instance is not
- // anymore registered as a service under any category.
- Runtime.getRuntime().removeShutdownHook(shutdown);
- shutdown = null;
- }
- }
- super.onDeregistration(registry, category);
- }
}
diff --git a/modules/library/referencing/src/main/java/org/geotools/referencing/operation/AuthorityBackedFactory.java b/modules/library/referencing/src/main/java/org/geotools/referencing/operation/AuthorityBackedFactory.java
index 88715a8..e02b282 100644
--- a/modules/library/referencing/src/main/java/org/geotools/referencing/operation/AuthorityBackedFactory.java
+++ b/modules/library/referencing/src/main/java/org/geotools/referencing/operation/AuthorityBackedFactory.java
@@ -304,7 +304,7 @@ public class AuthorityBackedFactory extends DefaultCoordinateOperationFactory
prepend = createOperation(sourceCRS, source).getMathTransform();
source = sourceCRS;
} finally {
- processing.set(Boolean.FALSE);
+ processing.remove();
} else {
prepend = null;
}
@@ -313,7 +313,7 @@ public class AuthorityBackedFactory extends DefaultCoordinateOperationFactory
append = createOperation(target, targetCRS).getMathTransform();
target = targetCRS;
} finally {
- processing.set(Boolean.FALSE);
+ processing.remove();
} else {
append = null;
}
diff --git a/modules/library/referencing/src/main/java/org/geotools/referencing/operation/DefaultMathTransformFactory.java b/modules/library/referencing/src/main/java/org/geotools/referencing/operation/DefaultMathTransformFactory.java
index 1abf672..c6a5fa6 100644
--- a/modules/library/referencing/src/main/java/org/geotools/referencing/operation/DefaultMathTransformFactory.java
+++ b/modules/library/referencing/src/main/java/org/geotools/referencing/operation/DefaultMathTransformFactory.java
@@ -130,7 +130,7 @@ public class DefaultMathTransformFactory extends ReferencingFactory implements M
/**
* The operation method for the last transform created.
*/
- private final ThreadLocal<OperationMethod> lastMethod;
+ private static final ThreadLocal<OperationMethod> lastMethod = new ThreadLocal<OperationMethod>();
/**
* A pool of math transform. This pool is used in order to
@@ -159,7 +159,6 @@ public class DefaultMathTransformFactory extends ReferencingFactory implements M
*/
private DefaultMathTransformFactory(final Class<?>[] categories) {
registry = new FactoryRegistry(Arrays.asList(categories));
- lastMethod = new ThreadLocal<OperationMethod>();
pool = CanonicalSet.newInstance(MathTransform.class);
}
@@ -708,4 +707,12 @@ public class DefaultMathTransformFactory extends ReferencingFactory implements M
}
arguments.out.flush();
}
+
+ /**
+ * Cleans up the thread local set in this thread. They can prevent web applications from
+ * proper shutdown
+ */
+ public static void cleanupThreadLocals() {
+ lastMethod.remove();
+ }
}
diff --git a/modules/library/referencing/src/main/java/org/geotools/referencing/wkt/Formattable.java b/modules/library/referencing/src/main/java/org/geotools/referencing/wkt/Formattable.java
index 49221ee..e7595c0 100644
--- a/modules/library/referencing/src/main/java/org/geotools/referencing/wkt/Formattable.java
+++ b/modules/library/referencing/src/main/java/org/geotools/referencing/wkt/Formattable.java
@@ -243,4 +243,13 @@ public class Formattable {
static void setIndentation(final int indentation) throws SecurityException {
Preferences.userNodeForPackage(Formattable.class).putInt(INDENTATION, indentation);
}
+
+ /**
+ * Cleans up the thread local set in this thread. They can prevent web applications from
+ * proper shutdown
+ */
+ public static void cleanupThreadLocals() {
+ FORMATTER.remove();
+ }
+
}
diff --git a/modules/library/referencing/src/main/java/org/geotools/referencing/wkt/Formatter.java b/modules/library/referencing/src/main/java/org/geotools/referencing/wkt/Formatter.java
index 6d9f05e..706ade2 100644
--- a/modules/library/referencing/src/main/java/org/geotools/referencing/wkt/Formatter.java
+++ b/modules/library/referencing/src/main/java/org/geotools/referencing/wkt/Formatter.java
@@ -884,6 +884,7 @@ public class Formatter {
lineChanged = false;
margin = 0;
}
+
/**
* Set the preferred indentation from the command line. This indentation is used by
diff --git a/modules/plugin/epsg-hsql/src/main/java/org/geotools/referencing/factory/epsg/ThreadedHsqlEpsgFactory.java b/modules/plugin/epsg-hsql/src/main/java/org/geotools/referencing/factory/epsg/ThreadedHsqlEpsgFactory.java
index 61c5d5d..f1b1972 100644
--- a/modules/plugin/epsg-hsql/src/main/java/org/geotools/referencing/factory/epsg/ThreadedHsqlEpsgFactory.java
+++ b/modules/plugin/epsg-hsql/src/main/java/org/geotools/referencing/factory/epsg/ThreadedHsqlEpsgFactory.java
@@ -41,6 +41,7 @@ import org.geotools.resources.i18n.Loggings;
import org.geotools.util.Version;
import org.geotools.util.logging.Logging;
import org.hsqldb.jdbc.jdbcDataSource;
+import org.opengis.referencing.FactoryException;
/**
@@ -226,6 +227,7 @@ public class ThreadedHsqlEpsgFactory extends ThreadedEpsgFactory {
url.append('/');
}
url.append(DATABASE_NAME);
+ url.append(";shutdown=true");
source.setDatabase(url.toString());
assert directory.equals(getDirectory(source)) : url;
}
------------------------------------------------------------------------------
_______________________________________________
Geotools-devel mailing list
Geotools-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geotools-devel
