Re: svn commit: r1232143 - /karaf/trunk/features/core/src/main/java/org/apache/karaf/features/internal/FeaturesServiceImpl.java

2012-09-11 Thread Guillaume Nodet
I just came across this bit of code which looks problematic to me.
The latch count down can only be used once, so if the region persistence
bundle is stopped for example, the getRegionsPersistence will never return.
Also the queue is not synchronized so that may cause issues ...
Why not using a simple optional reference in blueprint ? Given the code
just wait for a service to become available, that's exactly what the
blueprint proxies do.

On Mon, Jan 16, 2012 at 9:13 PM, ioca...@apache.org wrote:

 Author: iocanel
 Date: Mon Jan 16 20:13:49 2012
 New Revision: 1232143

 URL: http://svn.apache.org/viewvc?rev=1232143view=rev
 Log:
 [KARAF-1136] FeaturesServiceImpl will wait for at most 5 seconds for the
 RegionsPersistence service to become available, if need.

 Modified:

 karaf/trunk/features/core/src/main/java/org/apache/karaf/features/internal/FeaturesServiceImpl.java

 Modified:
 karaf/trunk/features/core/src/main/java/org/apache/karaf/features/internal/FeaturesServiceImpl.java
 URL:
 http://svn.apache.org/viewvc/karaf/trunk/features/core/src/main/java/org/apache/karaf/features/internal/FeaturesServiceImpl.java?rev=1232143r1=1232142r2=1232143view=diff

 ==
 ---
 karaf/trunk/features/core/src/main/java/org/apache/karaf/features/internal/FeaturesServiceImpl.java
 (original)
 +++
 karaf/trunk/features/core/src/main/java/org/apache/karaf/features/internal/FeaturesServiceImpl.java
 Mon Jan 16 20:13:49 2012
 @@ -47,6 +47,8 @@ import java.util.Queue;
  import java.util.Set;
  import java.util.TreeSet;
  import java.util.concurrent.CopyOnWriteArrayList;
 +import java.util.concurrent.CountDownLatch;
 +import java.util.concurrent.TimeUnit;
  import java.util.concurrent.atomic.AtomicBoolean;
  import java.util.jar.JarInputStream;
  import java.util.jar.Manifest;
 @@ -113,10 +115,12 @@ public class FeaturesServiceImpl impleme
  AtomicBoolean bootFeaturesInstalled = new AtomicBoolean();
  private ListFeaturesListener listeners = new
 CopyOnWriteArrayListFeaturesListener();
  private QueueRegionsPersistence regionsPersistenceQueue = new
 LinkedListRegionsPersistence();
 +private CountDownLatch regionsPersistenceLatch = new
 CountDownLatch(1);
  private ThreadLocalRepository repo = new ThreadLocalRepository();
  private EventAdminListener eventAdminListener;
  private final Object refreshLock = new Object();
  private long refreshTimeout = 5000;
 +private long regionsPersistenceTimeout = 5000;

  public FeaturesServiceImpl() {
  }
 @@ -172,8 +176,27 @@ public class FeaturesServiceImpl impleme
  }


 +/**
 + * Returns a RegionsPersistence service.
 + * @param timeout
 + * @return
 + */
 +protected RegionsPersistence getRegionsPersistence(long timeout) {
 +if (!regionsPersistenceQueue.isEmpty()) {
 +return regionsPersistenceQueue.peek();
 +} else {
 +try {
 +regionsPersistenceLatch.await(timeout,
 TimeUnit.MILLISECONDS);
 +} catch (InterruptedException e) {
 +LOGGER.warn(Interrupted while waittng for
 RegionsPersistence service,e);
 +}
 +return regionsPersistenceQueue.peek();
 +}
 +}
 +
  public void registerRegionsPersistence(RegionsPersistence
 regionsPersistence) {
  regionsPersistenceQueue.add(regionsPersistence);
 +regionsPersistenceLatch.countDown();
  }

  public void unregisterRegionsPersistence(RegionsPersistence
 regionsPersistence) {
 @@ -489,9 +512,13 @@ public class FeaturesServiceImpl impleme
  Bundle b = installBundleIfNeeded(state, bInfo,
 feature.getStartLevel(), verbose);
  bundles.add(b.getBundleId());
  state.bundleInfos.put(b.getBundleId(), bInfo);
 -RegionsPersistence regionsPersistence =
 regionsPersistenceQueue.peek();
 -if (region != null  state.installed.contains(b) 
 regionsPersistence != null) {
 -regionsPersistence.install(b, region);
 +if (region != null  state.installed.contains(b)) {
 +RegionsPersistence regionsPersistence =
 getRegionsPersistence(regionsPersistenceTimeout);
 +if (regionsPersistence != null) {
 +regionsPersistence.install(b, region);
 +} else {
 +throw new Exception(Unable to find
 RegionsPersistence service, while installing + feature.getName());
 +}
  }
  }
  state.features.put(feature, bundles);





-- 

Guillaume Nodet

Blog: http://gnodet.blogspot.com/

FuseSource, Integration everywhere
http://fusesource.com


Re: question about BootstrapLogManager

2012-09-11 Thread Andreas Pieber
Basically I see no reason not to do this; Where exactly would you like
to handle those exceptions? Do you have an idea/patch at hand for your
idea?

Kind regards,
Andreas

On Thu, Sep 6, 2012 at 3:47 AM, Heath Kesler heathkes...@gmail.com wrote:
 Hey guys,

 I am working on karaf-1748, and the consensus is that we should avoid 
 printing stack traces on a (permission denied) FileNotFoundException on 
 startup if the karaf.log is not accessible.  But in order to achieve this we 
 need to throw an exception from BootstrapLogManager.  Currently any exception 
 in that class just does a e.printStackTrace() then returns a null handler 
 (which also throws a NPE).  So this of course prints to the console.

 Is there any reason we should not throw an exception from that class and 
 handle it further up in the calling classes to avoid printing to the console 
 in this case (and possibly others)?  I believe the DefaultJDBCLock and the 
 SimpleFileLock would have to handle the exception along with a couple of 
 others.

 Cheers,
 Heath


Re: question about BootstrapLogManager

2012-09-11 Thread Heath Kesler
Thanks for the reply Andreas,

I am working on it, I hope to have a patch for review by the end of the week.  

Thanks
Heath

On Sep 11, 2012, at 2:54 AM, Andreas Pieber wrote:

 Basically I see no reason not to do this; Where exactly would you like
 to handle those exceptions? Do you have an idea/patch at hand for your
 idea?
 
 Kind regards,
 Andreas
 
 On Thu, Sep 6, 2012 at 3:47 AM, Heath Kesler heathkes...@gmail.com wrote:
 Hey guys,
 
 I am working on karaf-1748, and the consensus is that we should avoid 
 printing stack traces on a (permission denied) FileNotFoundException on 
 startup if the karaf.log is not accessible.  But in order to achieve this we 
 need to throw an exception from BootstrapLogManager.  Currently any 
 exception in that class just does a e.printStackTrace() then returns a null 
 handler (which also throws a NPE).  So this of course prints to the console.
 
 Is there any reason we should not throw an exception from that class and 
 handle it further up in the calling classes to avoid printing to the console 
 in this case (and possibly others)?  I believe the DefaultJDBCLock and the 
 SimpleFileLock would have to handle the exception along with a couple of 
 others.
 
 Cheers,
 Heath