Wow that is a tangle! I am still keen to have a GeoTools.cleanup() method; and perhaps a GeoTools.registerCleanup( Cleanup ) so that our "dependencies" can register themselves to be cleaned.
Boy I am stick of Factory SPI :-( Please commit on trunk as you say; we are fixing mistakes/sanity. Jody On 19/05/2010, at 11:48 PM, Andrea Aime wrote: > 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 ------------------------------------------------------------------------------ _______________________________________________ Geotools-devel mailing list Geotools-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geotools-devel
