diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index c6665e797da..00028025570 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -223,6 +223,9 @@ Bug Fixes a ClassCastException when a Map containing a non-String key is used. (Frank Wesemann, hossman) +* SOLR-1797: fix ConcurrentModificationException and potential memory + leaks in ResourceLoader. (yonik) + Other Changes ---------------------- diff --git a/solr/src/java/org/apache/solr/core/SolrCore.java b/solr/src/java/org/apache/solr/core/SolrCore.java index 49e72c34fe9..1a72d46cf6c 100644 --- a/solr/src/java/org/apache/solr/core/SolrCore.java +++ b/solr/src/java/org/apache/solr/core/SolrCore.java @@ -586,7 +586,7 @@ public final class SolrCore implements SolrInfoMBean { // Finally tell anyone who wants to know resourceLoader.inform( resourceLoader ); - resourceLoader.inform( this ); + resourceLoader.inform( this ); // last call before the latch is released. instance = this; // set singleton for backwards compatibility } catch (IOException e) { throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e); diff --git a/solr/src/java/org/apache/solr/core/SolrResourceLoader.java b/solr/src/java/org/apache/solr/core/SolrResourceLoader.java index 00296b450bc..114def4977e 100644 --- a/solr/src/java/org/apache/solr/core/SolrResourceLoader.java +++ b/solr/src/java/org/apache/solr/core/SolrResourceLoader.java @@ -69,13 +69,15 @@ public class SolrResourceLoader implements ResourceLoader private final String instanceDir; private String dataDir; - private final List waitingForCore = new ArrayList(); - private final List infoMBeans = new ArrayList(); - private final List waitingForResources = new ArrayList(); + private final List waitingForCore = Collections.synchronizedList(new ArrayList()); + private final List infoMBeans = Collections.synchronizedList(new ArrayList()); + private final List waitingForResources = Collections.synchronizedList(new ArrayList()); private static final Charset UTF_8 = Charset.forName("UTF-8"); private final Properties coreProperties; + private volatile boolean live; + /** *

* This loader will delegate to the context classloader when possible, @@ -399,18 +401,20 @@ public class SolrResourceLoader implements ResourceLoader throw new SolrException( SolrException.ErrorCode.SERVER_ERROR, "Error instantiating class: '" + clazz.getName()+"'", e, false ); } - - if( obj instanceof SolrCoreAware ) { - assertAwareCompatibility( SolrCoreAware.class, obj ); - waitingForCore.add( (SolrCoreAware)obj ); - } - if( obj instanceof ResourceLoaderAware ) { - assertAwareCompatibility( ResourceLoaderAware.class, obj ); - waitingForResources.add( (ResourceLoaderAware)obj ); - } - if (obj instanceof SolrInfoMBean){ - //TODO: Assert here? - infoMBeans.add((SolrInfoMBean) obj); + + if (!live) { + if( obj instanceof SolrCoreAware ) { + assertAwareCompatibility( SolrCoreAware.class, obj ); + waitingForCore.add( (SolrCoreAware)obj ); + } + if( obj instanceof ResourceLoaderAware ) { + assertAwareCompatibility( ResourceLoaderAware.class, obj ); + waitingForResources.add( (ResourceLoaderAware)obj ); + } + if (obj instanceof SolrInfoMBean){ + //TODO: Assert here? + infoMBeans.add((SolrInfoMBean) obj); + } } return obj; } @@ -431,12 +435,16 @@ public class SolrResourceLoader implements ResourceLoader throw new SolrException( SolrException.ErrorCode.SERVER_ERROR, "Error instantiating class: '" + clazz.getName()+"'", e, false ); } - //TODO: Does SolrCoreAware make sense here since in a multi-core context - // which core are we talking about ? - if( obj instanceof ResourceLoaderAware ) { - assertAwareCompatibility( ResourceLoaderAware.class, obj ); - waitingForResources.add( (ResourceLoaderAware)obj ); + + if (!live) { + //TODO: Does SolrCoreAware make sense here since in a multi-core context + // which core are we talking about ? + if( obj instanceof ResourceLoaderAware ) { + assertAwareCompatibility( ResourceLoaderAware.class, obj ); + waitingForResources.add( (ResourceLoaderAware)obj ); + } } + return obj; } @@ -460,18 +468,21 @@ public class SolrResourceLoader implements ResourceLoader "Error instantiating class: '" + clazz.getName()+"'", e, false ); } - if( obj instanceof SolrCoreAware ) { - assertAwareCompatibility( SolrCoreAware.class, obj ); - waitingForCore.add( (SolrCoreAware)obj ); - } - if( obj instanceof ResourceLoaderAware ) { - assertAwareCompatibility( ResourceLoaderAware.class, obj ); - waitingForResources.add( (ResourceLoaderAware)obj ); - } - if (obj instanceof SolrInfoMBean){ - //TODO: Assert here? - infoMBeans.add((SolrInfoMBean) obj); + if (!live) { + if( obj instanceof SolrCoreAware ) { + assertAwareCompatibility( SolrCoreAware.class, obj ); + waitingForCore.add( (SolrCoreAware)obj ); + } + if( obj instanceof ResourceLoaderAware ) { + assertAwareCompatibility( ResourceLoaderAware.class, obj ); + waitingForResources.add( (ResourceLoaderAware)obj ); + } + if (obj instanceof SolrInfoMBean){ + //TODO: Assert here? + infoMBeans.add((SolrInfoMBean) obj); + } } + return obj; } @@ -482,10 +493,24 @@ public class SolrResourceLoader implements ResourceLoader public void inform(SolrCore core) { this.dataDir = core.getDataDir(); - for( SolrCoreAware aware : waitingForCore ) { - aware.inform( core ); + + // make a copy to avoid potential deadlock of a callback calling newInstance and trying to + // add something to waitingForCore. + SolrCoreAware[] arr; + + while (waitingForCore.size() > 0) { + synchronized (waitingForCore) { + arr = waitingForCore.toArray(new SolrCoreAware[waitingForCore.size()]); + waitingForCore.clear(); + } + + for( SolrCoreAware aware : arr) { + aware.inform( core ); + } } - waitingForCore.clear(); + + // this is the last method to be called in SolrCore before the latch is released. + live = true; } /** @@ -493,10 +518,20 @@ public class SolrResourceLoader implements ResourceLoader */ public void inform( ResourceLoader loader ) { - for( ResourceLoaderAware aware : waitingForResources ) { - aware.inform( loader ); + + // make a copy to avoid potential deadlock of a callback adding to the list + ResourceLoaderAware[] arr; + + while (waitingForResources.size() > 0) { + synchronized (waitingForResources) { + arr = waitingForResources.toArray(new ResourceLoaderAware[waitingForResources.size()]); + waitingForResources.clear(); + } + + for( ResourceLoaderAware aware : arr) { + aware.inform(loader); + } } - waitingForResources.clear(); } /** @@ -504,10 +539,21 @@ public class SolrResourceLoader implements ResourceLoader * @param infoRegistry The Info Registry */ public void inform(Map infoRegistry) { - for (SolrInfoMBean bean : infoMBeans) { + // this can currently happen concurrently with requests starting and lazy components + // loading. Make sure infoMBeans doesn't change. + + SolrInfoMBean[] arr; + synchronized (infoMBeans) { + arr = infoMBeans.toArray(new SolrInfoMBean[infoMBeans.size()]); + waitingForResources.clear(); + } + + + for (SolrInfoMBean bean : arr) { infoRegistry.put(bean.getName(), bean); } } + /** * Determines the solrhome from the environment. * Tries JNDI (java:comp/env/solr/home) then system property (solr.solr.home);