On Thu, Sep 1, 2011 at 3:16 AM, Hadrian Zbarcea <hzbar...@gmail.com> wrote: > Yes, I agree with that, reason why Registries that use caches *are* and > should stay as Services. The PropertyEditorTypeConverter in particular > doesn't need to be a Service, but the EndpointRegisty, ConsumerCache and > ProducerCache should be and are Services. > > So you are correct about not leaving memory leaks, etc. Owners of caches > that hold Service(s) should and do shut them down before the cache clear() > since that's not the job of the cache itself (plus caches may not hold > Services in which case all the iteration over, dunno, hundreds, thousands of > object to check for (item instanceof Service) is kinda useless anyway. > Services are something that have a life cycle, the LRUCache is a handy data > structure. All in all I was quite careful about my changes, I hope I didn't > miss anything. I am sure that if anybody spots something he'll point it out > and we'll fix it. Needless to say, all tests pass after my changes and I am > fairly comfortable with the fix. > > Cheers, > Hadrian >
There is still a leaking problem. For example try run PropertyEditorTypeConverterIssueTest And then revert the change on PropertyEditorTypeConverterIssue to the previous commit (eg its a Service). And then for example do a System.out in doStop For example when I do this Starting org.apache.camel.impl.converter.PropertyEditorTypeConverter@6a8c436b Stopping org.apache.camel.impl.converter.PropertyEditorTypeConverter@6a8c436b with 0 in cache and 1 misses eg I can see that when Camel is shutdown this type converter is as well, where I then would be able to clear the caches. And thus not leak resources. > > > On 08/31/2011 03:55 PM, Claus Ibsen wrote: >> >> I wonder if there wont be a problem when you shutdown camel, or run >> Camel in a hot deployment environment. >> As before the service would ensure start|stop callbacks, where we >> could clear the caches when stopping. >> >> That is now gone, which means we can just hope the GC eventually will >> be able to reclaim the objects. >> >> So I suggest to use Service for classes that has caches or any map >> store which can hold possible a lot of data. >> Then being able to properly clean/stop those cachen when stopping >> Camel is a good citizen to avoid eating memory or leaking in any way. >> >> >> >> On Wed, Aug 31, 2011 at 7:10 PM,<hadr...@apache.org> wrote: >>> >>> Author: hadrian >>> Date: Wed Aug 31 17:10:53 2011 >>> New Revision: 1163705 >>> >>> URL: http://svn.apache.org/viewvc?rev=1163705&view=rev >>> Log: >>> CAMEL-4392. Fix side effect of LRUCache not a service. >>> >>> Modified: >>> >>> camel/trunk/camel-core/src/main/java/org/apache/camel/impl/converter/PropertyEditorTypeConverter.java >>> >>> camel/trunk/camel-core/src/test/java/org/apache/camel/component/bean/BeanComponentCustomCreateEndpointTest.java >>> >>> Modified: >>> camel/trunk/camel-core/src/main/java/org/apache/camel/impl/converter/PropertyEditorTypeConverter.java >>> URL: >>> http://svn.apache.org/viewvc/camel/trunk/camel-core/src/main/java/org/apache/camel/impl/converter/PropertyEditorTypeConverter.java?rev=1163705&r1=1163704&r2=1163705&view=diff >>> >>> ============================================================================== >>> --- >>> camel/trunk/camel-core/src/main/java/org/apache/camel/impl/converter/PropertyEditorTypeConverter.java >>> (original) >>> +++ >>> camel/trunk/camel-core/src/main/java/org/apache/camel/impl/converter/PropertyEditorTypeConverter.java >>> Wed Aug 31 17:10:53 2011 >>> @@ -22,11 +22,9 @@ import java.util.HashMap; >>> import java.util.Map; >>> >>> import org.apache.camel.Exchange; >>> -import org.apache.camel.Service; >>> import org.apache.camel.TypeConverter; >>> import org.apache.camel.util.LRUSoftCache; >>> import org.apache.camel.util.ObjectHelper; >>> -import org.apache.camel.util.ServiceHelper; >>> import org.slf4j.Logger; >>> import org.slf4j.LoggerFactory; >>> >>> @@ -36,14 +34,14 @@ import org.slf4j.LoggerFactory; >>> * >>> * @version >>> */ >>> -public class PropertyEditorTypeConverter implements TypeConverter, >>> Service { >>> +public class PropertyEditorTypeConverter implements TypeConverter { >>> >>> private static final Logger LOG = >>> LoggerFactory.getLogger(PropertyEditorTypeConverter.class); >>> // use a soft bound cache to avoid using too much memory in case a >>> lot of different classes >>> // is being converted to string >>> - private final Map<Class, Class> misses = new LRUSoftCache<Class, >>> Class>(1000); >>> + private final Map<Class<?>, Class<?>> misses = new >>> LRUSoftCache<Class<?>, Class<?>>(1000); >>> // we don't anticipate so many property editors so we have unbounded >>> map >>> - private final Map<Class, PropertyEditor> cache = new HashMap<Class, >>> PropertyEditor>(); >>> + private final Map<Class<?>, PropertyEditor> cache = new >>> HashMap<Class<?>, PropertyEditor>(); >>> >>> public<T> T convertTo(Class<T> type, Object value) { >>> // We can't convert null values since we can't figure out a >>> property >>> @@ -58,14 +56,14 @@ public class PropertyEditorTypeConverter >>> return ObjectHelper.cast(type, value); >>> } >>> >>> - Class key = type; >>> + Class<?> key = type; >>> PropertyEditor editor = lookupEditor(key); >>> if (editor != null) { >>> editor.setAsText(value.toString()); >>> return ObjectHelper.cast(type, editor.getValue()); >>> } >>> } else if (type == String.class) { >>> - Class key = value.getClass(); >>> + Class<?> key = value.getClass(); >>> PropertyEditor editor = lookupEditor(key); >>> if (editor != null) { >>> editor.setValue(value); >>> @@ -76,7 +74,7 @@ public class PropertyEditorTypeConverter >>> return null; >>> } >>> >>> - private PropertyEditor lookupEditor(Class type) { >>> + private PropertyEditor lookupEditor(Class<?> type) { >>> // check misses first >>> if (misses.containsKey(type)) { >>> LOG.trace("No previously found property editor for type: {}", >>> type); >>> @@ -115,14 +113,4 @@ public class PropertyEditorTypeConverter >>> public<T> T mandatoryConvertTo(Class<T> type, Exchange exchange, >>> Object value) { >>> return convertTo(type, value); >>> } >>> - >>> - public void start() throws Exception { >>> - ServiceHelper.startService(misses); >>> - } >>> - >>> - public void stop() throws Exception { >>> - cache.clear(); >>> - ServiceHelper.stopService(misses); >>> - } >>> - >>> } >>> >>> Modified: >>> camel/trunk/camel-core/src/test/java/org/apache/camel/component/bean/BeanComponentCustomCreateEndpointTest.java >>> URL: >>> http://svn.apache.org/viewvc/camel/trunk/camel-core/src/test/java/org/apache/camel/component/bean/BeanComponentCustomCreateEndpointTest.java?rev=1163705&r1=1163704&r2=1163705&view=diff >>> >>> ============================================================================== >>> --- >>> camel/trunk/camel-core/src/test/java/org/apache/camel/component/bean/BeanComponentCustomCreateEndpointTest.java >>> (original) >>> +++ >>> camel/trunk/camel-core/src/test/java/org/apache/camel/component/bean/BeanComponentCustomCreateEndpointTest.java >>> Wed Aug 31 17:10:53 2011 >>> @@ -47,11 +47,11 @@ public class BeanComponentCustomCreateEn >>> >>> public void testCreateEndpointUri() throws Exception { >>> BeanComponent bc = context.getComponent("bean", >>> BeanComponent.class); >>> - ProcessorEndpoint pe = bc.createEndpoint(new MyFooBean(), >>> "cheese"); >>> + ProcessorEndpoint pe = bc.createEndpoint(new MyFooBean(), >>> "bean:cheese"); >>> assertNotNull(pe); >>> >>> String uri = pe.getEndpointUri(); >>> - assertEquals("cheese", uri); >>> + assertEquals("bean:cheese", uri); >>> >>> Producer producer = pe.createProducer(); >>> Exchange exchange = producer.createExchange(); >>> >>> >>> >> >> >> > -- Claus Ibsen ----------------- FuseSource Email: cib...@fusesource.com Web: http://fusesource.com Twitter: davsclaus, fusenews Blog: http://davsclaus.blogspot.com/ Author of Camel in Action: http://www.manning.com/ibsen/
