Mailing-List: contact dev-help@camel.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@camel.apache.org
Received-SPF: pass (athena.apache.org: domain of claus.ibsen@gmail.com
 designates 209.85.223.172 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <c4b6255b34ba4f1f84606e5d9b6d9554@git.apache.org>
References: <c4b6255b34ba4f1f84606e5d9b6d9554@git.apache.org>
From: Claus Ibsen <claus.ibsen@gmail.com>
Date: Sat, 9 Nov 2013 15:50:59 +0100
Message-ID: 
 <CAGB5yNmVpWT9fBxPwjUAHR+0wGJm6wepPDAN8YrXzKzMivk+4A@mail.gmail.com>
Subject: Re: git commit: CAMEL-6948: Releasing a non-singleton producer by
 ProducerCache should not only stop the producer but also shutdown it as well
 if it is a ShutdownableService.
To: dev <dev@camel.apache.org>
Content-Type: text/plain; charset=ISO-8859-1

Hi

There is a stopAndShutdownService method on ServiceHelper in the util
package. Its maybe nicer to use to than the type cast. Then you can do
that in one command.

And this is also how we stop/start/shutdown services in a cleaner way.
And if you enabled TRACE/DEBUG logging the service helper can log what
happens as well.

On Fri, Nov 8, 2013 at 9:46 PM,  <bvahdat@apache.org> wrote:
> Updated Branches:
>   refs/heads/master f74ff2cf6 -> f744afd9c
>
>
> CAMEL-6948: Releasing a non-singleton producer by ProducerCache should not only stop the producer but also shutdown it as well if it is a ShutdownableService.
>
> Project: http://git-wip-us.apache.org/repos/asf/camel/repo
> Commit: http://git-wip-us.apache.org/repos/asf/camel/commit/f744afd9
> Tree: http://git-wip-us.apache.org/repos/asf/camel/tree/f744afd9
> Diff: http://git-wip-us.apache.org/repos/asf/camel/diff/f744afd9
>
> Branch: refs/heads/master
> Commit: f744afd9c39a1fc9261ad68dabe06a51a238fac9
> Parents: f74ff2c
> Author: Babak Vahdat <bvahdat@apache.org>
> Authored: Fri Nov 8 21:45:59 2013 +0100
> Committer: Babak Vahdat <bvahdat@apache.org>
> Committed: Fri Nov 8 21:45:59 2013 +0100
>
> ----------------------------------------------------------------------
>  .../org/apache/camel/impl/ProducerCache.java    |  6 +++
>  .../camel/impl/DefaultProducerCacheTest.java    | 53 +++++++++++++++++---
>  2 files changed, 51 insertions(+), 8 deletions(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/camel/blob/f744afd9/camel-core/src/main/java/org/apache/camel/impl/ProducerCache.java
> ----------------------------------------------------------------------
> diff --git a/camel-core/src/main/java/org/apache/camel/impl/ProducerCache.java b/camel-core/src/main/java/org/apache/camel/impl/ProducerCache.java
> index 6b292c0..b35eca5 100644
> --- a/camel-core/src/main/java/org/apache/camel/impl/ProducerCache.java
> +++ b/camel-core/src/main/java/org/apache/camel/impl/ProducerCache.java
> @@ -30,6 +30,7 @@ import org.apache.camel.Processor;
>  import org.apache.camel.Producer;
>  import org.apache.camel.ProducerCallback;
>  import org.apache.camel.ServicePoolAware;
> +import org.apache.camel.ShutdownableService;
>  import org.apache.camel.processor.UnitOfWorkProducer;
>  import org.apache.camel.spi.ServicePool;
>  import org.apache.camel.support.ServiceSupport;
> @@ -137,6 +138,11 @@ public class ProducerCache extends ServiceSupport {
>          } else if (!producer.isSingleton()) {
>              // stop non singleton producers as we should not leak resources
>              producer.stop();
> +
> +            // shutdown as well in case the producer is shutdownable
> +            if (producer instanceof ShutdownableService) {
> +                ShutdownableService.class.cast(producer).shutdown();
> +            }
>          }
>      }
>
>
> http://git-wip-us.apache.org/repos/asf/camel/blob/f744afd9/camel-core/src/test/java/org/apache/camel/impl/DefaultProducerCacheTest.java
> ----------------------------------------------------------------------
> diff --git a/camel-core/src/test/java/org/apache/camel/impl/DefaultProducerCacheTest.java b/camel-core/src/test/java/org/apache/camel/impl/DefaultProducerCacheTest.java
> index 5ad0f42..803302c 100644
> --- a/camel-core/src/test/java/org/apache/camel/impl/DefaultProducerCacheTest.java
> +++ b/camel-core/src/test/java/org/apache/camel/impl/DefaultProducerCacheTest.java
> @@ -30,7 +30,8 @@ import org.apache.camel.Producer;
>   */
>  public class DefaultProducerCacheTest extends ContextTestSupport {
>
> -    private static final AtomicInteger COUNTER = new AtomicInteger();
> +    private final AtomicInteger stopCounter = new AtomicInteger();
> +    private final AtomicInteger shutdownCounter = new AtomicInteger();
>
>      public void testCacheProducerAcquireAndRelease() throws Exception {
>          ProducerCache cache = new ProducerCache(this, context);
> @@ -56,7 +57,7 @@ public class DefaultProducerCacheTest extends ContextTestSupport {
>          assertEquals("Size should be 0", 0, cache.size());
>
>          for (int i = 0; i < 8; i++) {
> -            Endpoint e = new MyEndpoint(i);
> +            Endpoint e = new MyEndpoint(true, i);
>              Producer p = cache.acquireProducer(e);
>              cache.releaseProducer(e, p);
>          }
> @@ -64,19 +65,50 @@ public class DefaultProducerCacheTest extends ContextTestSupport {
>          assertEquals("Size should be 5", 5, cache.size());
>
>          // should have stopped the 3 evicted
> -        assertEquals(3, COUNTER.get());
> +        assertEquals(3, stopCounter.get());
>
>          cache.stop();
>
>          // should have stopped all 8
> -        assertEquals(8, COUNTER.get());
> +        assertEquals(8, stopCounter.get());
> +    }
> +
> +    public void testReleaseProducerInvokesStopAndShutdownByNonSingeltonProducers() throws Exception {
> +        ProducerCache cache = new ProducerCache(this, context, 1);
> +        cache.start();
> +
> +        assertEquals("Size should be 0", 0, cache.size());
> +
> +        for (int i = 0; i < 3; i++) {
> +            Endpoint e = new MyEndpoint(false, i);
> +            Producer p = cache.acquireProducer(e);
> +            cache.releaseProducer(e, p);
> +        }
> +
> +        assertEquals("Size should be 0", 0, cache.size());
> +
> +        // should have stopped all 3
> +        assertEquals(3, stopCounter.get());
> +
> +        // should have shutdown all 3
> +        assertEquals(3, shutdownCounter.get());
> +
> +        cache.stop();
> +
> +        // no more stop after stopping the cache
> +        assertEquals(3, stopCounter.get());
> +
> +        // no more shutdown after stopping the cache
> +        assertEquals(3, shutdownCounter.get());
>      }
>
>      private final class MyEndpoint extends DefaultEndpoint {
>
> -        private int number;
> +        private final boolean isSingleton;
> +        private final int number;
>
> -        private MyEndpoint(int number) {
> +        private MyEndpoint(boolean isSingleton, int number) {
> +            this.isSingleton = isSingleton;
>              this.number = number;
>          }
>
> @@ -92,7 +124,7 @@ public class DefaultProducerCacheTest extends ContextTestSupport {
>
>          @Override
>          public boolean isSingleton() {
> -            return true;
> +            return isSingleton;
>          }
>
>          @Override
> @@ -114,7 +146,12 @@ public class DefaultProducerCacheTest extends ContextTestSupport {
>
>          @Override
>          protected void doStop() throws Exception {
> -            COUNTER.incrementAndGet();
> +            stopCounter.incrementAndGet();
> +        }
> +
> +        @Override
> +        protected void doShutdown() throws Exception {
> +            shutdownCounter.incrementAndGet();
>          }
>      }
>
>


-- 
Claus Ibsen
-----------------
Red Hat, Inc.
Email: cibsen@redhat.com
Twitter: davsclaus
Blog: http://davsclaus.com
Author of Camel in Action: http://www.manning.com/ibsen