[seek-dev] Questions about implementation of org.kepler.objectmanager.cache.DataCacheObject

[Kevin Ruland]

Hi all,

I have some questions about the o.k.objectmanager.cache.DataCacheObject
implementation relating to the use of a spin lock in
getDataItemFromEcoGrid().

The critical facts here are:

protected boolean needSynchronize = true;
private static boolean lockEcoGrid = false;

  protected void getDataItemFromEcoGrid(String endPoint, String identifier)
  {

    // create a ecogrid client object and get the full record from the
    // client
    dbg.print("Get " + identifier + " from " + endPoint, 2);
    if(needSynchronize)
    {
      while(lockEcoGrid)
      {
        dbg.print("sleep " + identifier, 2);
        try
        {
          Thread.sleep(500);
        }
        catch(Exception e)
        {

        }
      }
      synchronized(this)
      {
        lockEcoGrid = true;
      }

    }

//*********** Here work is done

    lockEcoGrid = false;
    dbg.print("realse lock from " + identifier, 2);

  }

I suspect the intention of the lockEcoGrid flag it to prevent two
threads from running the block of code which does actual work.  However,
the code does not do this.  In fact, the spinlock and synchronization
code does nothing at all.  Certainly the while loop will spin and wait,
but the synchronized block does not prevent multiple threads from
concurrently operating in the following block.  Suppose  we have thread
1 and thread 2 executing and lockEcoGrid is false.  Both threads can
fall through to the synchronized statement.  Now thread 1 gets to
execute the synchronized block, when it's finished, suppose it is
suspended.  Then thread 2 gets to execute the synchronized block.  Now
both threads can execute the code in "Here work is done".

The only class variable here is lockEcoGrid.  So I have to wonder,
exactly what is being locked? If some remote resource (ecogrid server)
is being locked then this locking should be done in the
EcogridFactoryQueryClient class.  If this is code held over from a
failed attempt to manage the remote resource, then it should be
removed.  One final consideration is the remote resources can
potentially be used by multiple Kepler instances and even two JVMs on
the same host do not share synchronization semiphores.

Another question I have about this class is why it has a member called
"getDataFromEcoGrid()"?  Does this really fit with the class' other
methods which is about managing a file based cache system?  The method
is used by both EcogridMetaDataCacheItem, and EcogridDataCacheItem. 
Perhaps it is better for those two objects to share another ancestor. 
And finally, EcogridQueryDataCacheItem has almost idential code in its
doWork() method so it might benefit from this method as well.

Kevin