Re: svn commit: r467846 - in /xml/xindice/trunk/java/src/org/apache/xindice/core: Collection.java Database.java DatabaseShutdownHandler.java

In the DatabaseShudownHandler.run method the synchronized block needs to be
outside the loop. Its possible for the Map to be modified by a different
thread and that will throw a ConcurrentModificationExpection from the
iteration.

Todd Byrne

On 10/25/06, [email protected] <[email protected]> wrote:
>
> Author: vgritsenko
> Date: Wed Oct 25 20:03:45 2006
> New Revision: 467846
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=467846
> Log:
> cleanup shutdown hook.
> remove lock on shutdown.
>
> Modified:
>     xml/xindice/trunk/java/src/org/apache/xindice/core/Collection.java
>     xml/xindice/trunk/java/src/org/apache/xindice/core/Database.java
>
>     xml/xindice/trunk/java/src/org/apache/xindice/core/DatabaseShutdownHandler.java
>
> Modified:
> xml/xindice/trunk/java/src/org/apache/xindice/core/Collection.java
> URL:
> http://svn.apache.org/viewvc/xml/xindice/trunk/java/src/org/apache/xindice/core/Collection.java?view=diff&rev=467846&r1=467845&r2=467846
>
> ==============================================================================
> --- xml/xindice/trunk/java/src/org/apache/xindice/core/Collection.java
> (original)
> +++ xml/xindice/trunk/java/src/org/apache/xindice/core/Collection.java Wed
> Oct 25 20:03:45 2006
> @@ -609,6 +609,12 @@
>          MetaData meta = metacol.getDocumentMeta(this, id);
>
>          /*
> +        FIXME It is more efficient to store (and retrieve)
> created/modified timestamps
> +              from the Record itself instead of storing them in the
> separate MetaData
> +              object. Storing in the Record avoids writing two documents
> on each update
> +              (Document itself and its MetaData).
> +              Retrieval of the timestamps from Record can be implemented
> via TimeRecord.
> +
>          TimeRecord rec = null;
>          if( null == meta || !meta.hasContext() )
>             rec = getDatabase().getTime(path);
>
> Modified: xml/xindice/trunk/java/src/org/apache/xindice/core/Database.java
> URL:
> http://svn.apache.org/viewvc/xml/xindice/trunk/java/src/org/apache/xindice/core/Database.java?view=diff&rev=467846&r1=467845&r2=467846
>
> ==============================================================================
> --- xml/xindice/trunk/java/src/org/apache/xindice/core/Database.java
> (original)
> +++ xml/xindice/trunk/java/src/org/apache/xindice/core/Database.java Wed
> Oct 25 20:03:45 2006
> @@ -66,6 +66,7 @@
>          // changes to disk.
>          DBObserver.setInstance(new DatabaseChangeObserver());
>      }
> +
>      /**
>       * This will return an instance of a Database for the given
>       * name if one has already been loaded, otherwise it will
> @@ -156,6 +157,7 @@
>          shutdownHandler.registerDatabase(this);
>          closed = false;
>      }
> +
>      /**
>       * Checks to see if it has been closed to insure that it doesn't try
> to do it
>       * twice.
> @@ -164,13 +166,13 @@
>       */
>      protected synchronized boolean close(boolean removeFromShutdown)
> throws DBException {
>
> -        if(removeFromShutdown) {
> +        if (removeFromShutdown) {
>              // we have already been closed so no need to do this again.
>              shutdownHandler.removeDatabase(this);
>          }
> +
>          // check to see if we have already been closed.
> -        if(!closed)
> -        {
> +        if (!closed) {
>              if(log.isDebugEnabled()) {
>                  log.debug("Shutting down database: '" + getName() + "'");
>              }
> @@ -181,17 +183,18 @@
>              // Release database lock
>              try {
>                  this.lock.close();
> -            }
> -            catch(IOException e) {
> +                new File(getCollectionRoot(), "db.lock").delete();
> +            } catch (Exception e) {
>                  // Ignore IO exception
>              }
> -            this.lock=null;
> +            this.lock = null;
>
> -            synchronized(databases) {
> +            synchronized (databases) {
>                  databases.remove(getName());
>              }
>              closed = true;
>          }
> +
>          return true;
>      }
>      /**
>
> Modified:
> xml/xindice/trunk/java/src/org/apache/xindice/core/DatabaseShutdownHandler.java
> URL:
> http://svn.apache.org/viewvc/xml/xindice/trunk/java/src/org/apache/xindice/core/DatabaseShutdownHandler.java?view=diff&rev=467846&r1=467845&r2=467846
>
> ==============================================================================
> ---
> xml/xindice/trunk/java/src/org/apache/xindice/core/DatabaseShutdownHandler.java
> (original)
> +++
> xml/xindice/trunk/java/src/org/apache/xindice/core/DatabaseShutdownHandler.java
> Wed Oct 25 20:03:45 2006
> @@ -16,9 +16,10 @@
> */
> package org.apache.xindice.core;
>
> -import java.util.Hashtable;
> +import java.util.HashSet;
> import java.util.Iterator;
> import java.util.Map;
> +import java.util.Set;
>
> /**
>   * Going to handle the JVM Shutdown hook and insuring clean shutdown
> @@ -26,49 +27,57 @@
>   *
>   * <a href="mailto:[email protected]">Todd Byrne</a>
>   */
> -public class DatabaseShutdownHandler implements Runnable{
> +public class DatabaseShutdownHandler implements Runnable {
> +
> +    private final Set databases;
>
> -    private Hashtable databases;
>
>      public DatabaseShutdownHandler() {
> -        databases = new Hashtable();
> +        databases = new HashSet();
>          Runtime.getRuntime().addShutdownHook(new Thread(this));
>      }
> +
>      /**
>       * @param db database to register
>       */
>      public void registerDatabase(Database db) {
> -        databases.put(db, Boolean.TRUE);
> +        synchronized (databases) {
> +            databases.add(db);
> +        }
>      }
> +
>      /**
>       * @param db removed the database from the hashtable
>       */
>      public void removeDatabase(Database db) {
> -        databases.remove(db);
> +        synchronized (databases) {
> +            databases.remove(db);
> +        }
>      }
> +
>      /**
>       * Cleans up any unclosed databases before the JVM exits.
>       */
>      public void run() {
> -        synchronized(databases) {
> -            Iterator dbs = databases.entrySet().iterator();
> -            while(dbs.hasNext())
> -            {
> -                Database db = (Database)((Map.Entry)dbs.next()).getKey();
> +        synchronized (databases) {
> +            Iterator dbs = databases.iterator();
> +            while (dbs.hasNext()) {
> +                Database db = (Database) dbs.next();
>
>                  // synch again on the db object just incase a close
> operation
>                  // is happening on a different thread
> -                synchronized(db) {
> +                synchronized (db) {
>                      // call close but don't bother removing the db from
> the database
>                      // table
>                      try {
>                          db.close(false);
> -                        System.out.println("Dirty close on: " +
> db.getName());
> -                    }
> -                    catch(DBException ex) {
> +                        System.out.println("SHUTDOWN Dirty close on: " +
> db.getName());
> +                    } catch (DBException e) {
> +                        System.out.println("SHUTDOWN Failed to close: " +
> db.getName() + ": " + e);
>                      }
>                  }
>              }
> +
>              databases.clear();
>          }
>      }
>
>
>