roller-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From agillil...@apache.org
Subject svn commit: r495640 - in /incubator/roller/trunk: metadata/database/ src/org/apache/roller/business/hibernate/ src/org/apache/roller/business/pings/ src/org/apache/roller/business/runnable/ src/org/apache/roller/planet/tasks/ tests/org/apache/roller/bu...
Date Fri, 12 Jan 2007 16:58:11 GMT
Author: agilliland
Date: Fri Jan 12 08:58:10 2007
New Revision: 495640

URL: http://svn.apache.org/viewvc?view=rev&rev=495640
Log:
Fixes for race conditions in lock acquisition via HibernateThreadManagerImpl from Jira issue
ROL-1306.  A number of fixes are included in this commit ...

1. Added a database uniqueness constraint on the roller_tasklock.name column to
prevent any possibility of there being duplicate insertions for a given task lease.

2. Updated the acquireLock() method to use a more robust acquisition method based on a single
sql update statement as suggested by Elias.  This should adequately eliminate race conditions
when 2 or more members of a cluster attempt to acquire a lock at the same time.

3. Updated the acquireLock() method to perform all lease activities based on database time,
which should negate any potential clock skew issues between cluster
members.

4. Introduced a new clientId attribute for a task which makes it possible for each member
of a cluster to be uniquely identified with regards to ownership of a
lease.



Modified:
    incubator/roller/trunk/metadata/database/310-to-320-migration.vm
    incubator/roller/trunk/metadata/database/createdb.vm
    incubator/roller/trunk/src/org/apache/roller/business/hibernate/HibernateThreadManagerImpl.java
    incubator/roller/trunk/src/org/apache/roller/business/pings/PingQueueTask.java
    incubator/roller/trunk/src/org/apache/roller/business/runnable/ResetHitCountsTask.java
    incubator/roller/trunk/src/org/apache/roller/business/runnable/RollerTask.java
    incubator/roller/trunk/src/org/apache/roller/business/runnable/ThreadManager.java
    incubator/roller/trunk/src/org/apache/roller/business/runnable/TurnoverReferersTask.java
    incubator/roller/trunk/src/org/apache/roller/planet/tasks/RefreshEntriesTask.java
    incubator/roller/trunk/src/org/apache/roller/planet/tasks/SyncWebsitesTask.java
    incubator/roller/trunk/tests/org/apache/roller/business/TaskLockTest.java
    incubator/roller/trunk/web/WEB-INF/classes/roller.properties

Modified: incubator/roller/trunk/metadata/database/310-to-320-migration.vm
URL: http://svn.apache.org/viewvc/incubator/roller/trunk/metadata/database/310-to-320-migration.vm?view=diff&rev=495640&r1=495639&r2=495640
==============================================================================
--- incubator/roller/trunk/metadata/database/310-to-320-migration.vm (original)
+++ incubator/roller/trunk/metadata/database/310-to-320-migration.vm Fri Jan 12 08:58:10 2007
@@ -33,6 +33,12 @@
 -- add new status option 'SCHEDULED' for future published entries
 update weblogentry set status = 'SCHEDULED' where pubtime > now();
 
+-- add new client column to roller_tasklock table
+#addColumnNull("roller_tasklock" "client" "varchar(255)")
+
+-- add a uniqueness constraint on roller_tasklock.name
+alter table roller_tasklock add constraint rtl_name_uq unique ( name$!INDEXSIZE );
+
 -- some various indexes to improve performance
 create index rhc_dailyhits_idx on roller_hitcounts( dailyhits );
 create index we_combo1_idx on weblogentry(status, pubtime, websiteid);

Modified: incubator/roller/trunk/metadata/database/createdb.vm
URL: http://svn.apache.org/viewvc/incubator/roller/trunk/metadata/database/createdb.vm?view=diff&rev=495640&r1=495639&r2=495640
==============================================================================
--- incubator/roller/trunk/metadata/database/createdb.vm (original)
+++ incubator/roller/trunk/metadata/database/createdb.vm Fri Jan 12 08:58:10 2007
@@ -398,8 +398,10 @@
     islocked        $BOOLEAN_SQL_TYPE_FALSE,
     timeacquired    $TIMESTAMP_SQL_TYPE_NULL,
     timeleased	    integer,
-    lastrun         $TIMESTAMP_SQL_TYPE_NULL
+    lastrun         $TIMESTAMP_SQL_TYPE_NULL,
+    client          varchar(255)
 );
+alter table roller_tasklock add constraint rtl_name_uq unique ( name$!INDEXSIZE );
 create index rtl_taskname_idx on roller_tasklock( name );
 
 create table roller_hitcounts (

Modified: incubator/roller/trunk/src/org/apache/roller/business/hibernate/HibernateThreadManagerImpl.java
URL: http://svn.apache.org/viewvc/incubator/roller/trunk/src/org/apache/roller/business/hibernate/HibernateThreadManagerImpl.java?view=diff&rev=495640&r1=495639&r2=495640
==============================================================================
--- incubator/roller/trunk/src/org/apache/roller/business/hibernate/HibernateThreadManagerImpl.java
(original)
+++ incubator/roller/trunk/src/org/apache/roller/business/hibernate/HibernateThreadManagerImpl.java
Fri Jan 12 08:58:10 2007
@@ -18,7 +18,6 @@
 
 package org.apache.roller.business.hibernate;
 
-import java.util.Calendar;
 import java.util.Date;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
@@ -29,6 +28,7 @@
 import org.apache.roller.pojos.TaskLockData;
 import org.hibernate.Criteria;
 import org.hibernate.HibernateException;
+import org.hibernate.Query;
 import org.hibernate.Session;
 import org.hibernate.criterion.Expression;
 
@@ -57,53 +57,60 @@
     
     /**
      * Try to aquire a lock for a given RollerTask.
-     *
-     * Remember, locks are only given if ...
-     *   1. the task is not currently locked and it's past the next scheduled
-     *      run time for the particular task
-     *   2. the task *is* locked, but its lease has expired
      */
     public boolean acquireLock(RollerTask task) {
         
-        boolean lockAcquired = false;
-        
+        // query for existing lease record first
         TaskLockData taskLock = null;
         try {
             taskLock = this.getTaskLockByName(task.getName());
             
-            // null here just means hasn't been initialized yet
             if(taskLock == null) {
+                // insert an empty record, then we will actually acquire the
+                // lease below using an update statement 
                 taskLock = new TaskLockData();
                 taskLock.setName(task.getName());
-                taskLock.setLocked(false);
+                taskLock.setTimeAquired(new Date(0));
+                taskLock.setTimeLeased(0);
+                
+                // save it and flush
+                this.saveTaskLock(taskLock);
+                RollerFactory.getRoller().flush();
             }
+            
         } catch (RollerException ex) {
-            log.warn("Error getting TaskLockData", ex);
+            log.warn("Error getting or inserting TaskLockData", ex);
             return false;
         }
         
-        Date now = new Date();
-        Date nextRun = taskLock.getNextRun(task.getInterval());
-        if( !taskLock.isLocked() && (nextRun == null || now.after(nextRun))) {
-            
-            // set appropriate values for TaskLock and save it
-            taskLock.setLocked(true);
-            taskLock.setTimeAquired(now);
-            taskLock.setTimeLeased(task.getLeaseTime());
-            taskLock.setLastRun(now);
+        // try to acquire lease
+        try {
+            Session session = ((HibernatePersistenceStrategy)this.strategy).getSession();
+            String queryHQL = "update TaskLockData "+
+                    "set client=:client, timeacquired=current_timestamp(), timeleased=:timeleased
"+
+                    "where name=:name and timeacquired=:timeacquired "+
+                    "and :leaseends < current_timestamp()";
+            Query query = session.createQuery(queryHQL);
+            query.setString("client", task.getClientId());
+            query.setString("timeleased", ""+task.getLeaseTime());
+            query.setString("name", task.getName());
+            query.setTimestamp("timeacquired", taskLock.getTimeAquired());
+            query.setTimestamp("leaseends", new Date(taskLock.getTimeAquired().getTime()+(60000*taskLock.getTimeLeased())));
+            int result = query.executeUpdate();
             
-            try {
-                // save it *and* flush
-                this.saveTaskLock(taskLock);
-                RollerFactory.getRoller().flush();
-                lockAcquired = true;
-            } catch (RollerException ex) {
-                log.warn("Error saving TaskLockData", ex);
-                lockAcquired = false;
+            // this may not be needed
+            RollerFactory.getRoller().flush();
+            
+            if(result == 1) {
+                return true;
             }
+            
+        } catch (Exception e) {
+            log.warn("Error obtaining lease, assuming race condition.", e);
+            return false;
         }
         
-        return lockAcquired;
+        return false;
     }
     
     
@@ -112,61 +119,43 @@
      */
     public boolean releaseLock(RollerTask task) {
         
-        boolean lockReleased = false;
-        
+        // query for existing lease record first
         TaskLockData taskLock = null;
         try {
             taskLock = this.getTaskLockByName(task.getName());
+            
+            if(taskLock == null) {
+                return false;
+            }
+            
         } catch (RollerException ex) {
             log.warn("Error getting TaskLockData", ex);
             return false;
         }
         
-        if(taskLock != null && taskLock.isLocked()) {
-            // set appropriate values for TaskLock and save it
-            Date now = new Date();
-            taskLock.setLocked(false);
-            
-            try {
-                // save it *and* flush
-                this.saveTaskLock(taskLock);
-                RollerFactory.getRoller().flush();
-                lockReleased = true;
-            } catch (RollerException ex) {
-                log.warn("Error saving TaskLockData", ex);
-                lockReleased = false;
-            }
-        } else if(taskLock != null && !taskLock.isLocked()) {
-            // if lock is already released then don't fret about it
-            lockReleased = true;
-        }
-        
-        return lockReleased;
-    }
-    
-    
-    /**
-     * Is a task currently locked?
-     */
-    public boolean isLocked(RollerTask task) {
-        
-        // default is "true"!
-        boolean locked = true;
-        
+        // try to release lease, just set lease time to 0
         try {
-            TaskLockData taskLock = this.getTaskLockByName(task.getName());
-            if(taskLock != null) {
-                locked = taskLock.isLocked();
-            } else {
-                // if taskLock is null, but we didn't get an exception then
-                // that means this lock hasn't been initialized yet
-                locked = false;
+            Session session = ((HibernatePersistenceStrategy)this.strategy).getSession();
+            String queryHQL = "update TaskLockData set timeLeased=0 "+
+                    "where name=:name and client=:client";
+            Query query = session.createQuery(queryHQL);
+            query.setString("name", task.getName());
+            query.setString("client", task.getClientId());
+            int result = query.executeUpdate();
+            
+            // this may not be needed
+            RollerFactory.getRoller().flush();
+            
+            if(result == 1) {
+                return true;
             }
-        } catch (RollerException ex) {
-            log.warn("Error getting TaskLockData", ex);
+            
+        } catch (Exception e) {
+            log.warn("Error releasing lease.", e);
+            return false;
         }
         
-        return locked;
+        return false;
     }
     
     

Modified: incubator/roller/trunk/src/org/apache/roller/business/pings/PingQueueTask.java
URL: http://svn.apache.org/viewvc/incubator/roller/trunk/src/org/apache/roller/business/pings/PingQueueTask.java?view=diff&rev=495640&r1=495639&r2=495640
==============================================================================
--- incubator/roller/trunk/src/org/apache/roller/business/pings/PingQueueTask.java (original)
+++ incubator/roller/trunk/src/org/apache/roller/business/pings/PingQueueTask.java Fri Jan
12 08:58:10 2007
@@ -41,6 +41,10 @@
     
     private static Log log = LogFactory.getLog(PingQueueTask.class);
     
+    // a unique id for this specific task instance
+    // this is meant to be unique for each client in a clustered environment
+    private String clientId = null;
+    
     // a String description of when to start this task
     private String startTimeDesc = "immediate";
     
@@ -55,6 +59,10 @@
         return "PingQueueTask";
     }
     
+    public String getClientId() {
+        return clientId;
+    }
+    
     public Date getStartTime(Date currentTime) {
         return getAdjustedTime(currentTime, startTimeDesc);
     }
@@ -72,6 +80,12 @@
         
         // get relevant props
         Properties props = this.getTaskProperties();
+        
+        // extract clientId
+        String client = props.getProperty("clientId");
+        if(client != null) {
+            this.clientId = client;
+        }
         
         // extract start time
         String startTimeStr = props.getProperty("startTime");

Modified: incubator/roller/trunk/src/org/apache/roller/business/runnable/ResetHitCountsTask.java
URL: http://svn.apache.org/viewvc/incubator/roller/trunk/src/org/apache/roller/business/runnable/ResetHitCountsTask.java?view=diff&rev=495640&r1=495639&r2=495640
==============================================================================
--- incubator/roller/trunk/src/org/apache/roller/business/runnable/ResetHitCountsTask.java
(original)
+++ incubator/roller/trunk/src/org/apache/roller/business/runnable/ResetHitCountsTask.java
Fri Jan 12 08:58:10 2007
@@ -34,6 +34,10 @@
     
     private static Log log = LogFactory.getLog(ResetHitCountsTask.class);
     
+    // a unique id for this specific task instance
+    // this is meant to be unique for each client in a clustered environment
+    private String clientId = null;
+    
     // a String description of when to start this task
     private String startTimeDesc = "startOfDay";
     
@@ -48,6 +52,10 @@
         return "ResetHitCountsTask";
     }
     
+    public String getClientId() {
+        return clientId;
+    }
+    
     public Date getStartTime(Date currentTime) {
         return getAdjustedTime(currentTime, startTimeDesc);
     }
@@ -65,6 +73,12 @@
         
         // get relevant props
         Properties props = this.getTaskProperties();
+        
+        // extract clientId
+        String client = props.getProperty("clientId");
+        if(client != null) {
+            this.clientId = client;
+        }
         
         // extract start time
         String startTimeStr = props.getProperty("startTime");

Modified: incubator/roller/trunk/src/org/apache/roller/business/runnable/RollerTask.java
URL: http://svn.apache.org/viewvc/incubator/roller/trunk/src/org/apache/roller/business/runnable/RollerTask.java?view=diff&rev=495640&r1=495639&r2=495640
==============================================================================
--- incubator/roller/trunk/src/org/apache/roller/business/runnable/RollerTask.java (original)
+++ incubator/roller/trunk/src/org/apache/roller/business/runnable/RollerTask.java Fri Jan
12 08:58:10 2007
@@ -60,6 +60,16 @@
     
     
     /**
+     * Get the unique id representing a specific instance of a task.  This is
+     * important for tasks being run in a clustered environment so that a 
+     * lease can be associated with a single cluster member.
+     *
+     * @return The unique client identifier for this task instance.
+     */
+    public abstract String getClientId();
+    
+    
+    /**
      * When should this task be started?  The task is given the current time
      * so that it may determine a start time relative to the current time, 
      * such as the end of the day or hour.
@@ -112,36 +122,21 @@
      */
     public final void run() {
         
-        ThreadManager mgr = null;
-        try {
-            mgr = RollerFactory.getRoller().getThreadManager();
-        } catch (Exception ex) {
-            log.fatal("Unable to obtain ThreadManager", ex);
-            return;
-        }
+        ThreadManager mgr = RollerFactory.getRoller().getThreadManager();
         
         boolean lockAcquired = false;
         try {
+            log.debug("Attempting to acquire lock");
             
-            if(!mgr.isLocked(this)) {
-                
-                log.debug("Attempting to acquire lock");
-                
-                lockAcquired = mgr.acquireLock(this);
-                
-                // now if we have a lock then run the task
-                if(lockAcquired) {
-                    log.debug("Lock acquired, running task");
-                    this.runTask();
-                } else {
-                    log.debug("Lock NOT acquired, cannot continue");
-                    
-                    // when we don't have a lock we can't continue, so bail
-                    return;
-                }
-                
+            lockAcquired = mgr.acquireLock(this);
+            
+            // now if we have a lock then run the task
+            if(lockAcquired) {
+                log.debug("Lock acquired, running task");
+                this.runTask();
             } else {
-                log.debug("Task already locked, nothing to do");
+                log.debug("Lock NOT acquired, cannot continue");
+                return;
             }
             
         } catch (Exception ex) {
@@ -191,6 +186,9 @@
                         RollerConfig.getProperty(key));
             }
         }
+        
+        // special addition for clientId property that applies to all tasks
+        taskProps.setProperty("clientId", RollerConfig.getProperty("tasks.clientId"));
         
         return taskProps;
     }

Modified: incubator/roller/trunk/src/org/apache/roller/business/runnable/ThreadManager.java
URL: http://svn.apache.org/viewvc/incubator/roller/trunk/src/org/apache/roller/business/runnable/ThreadManager.java?view=diff&rev=495640&r1=495639&r2=495640
==============================================================================
--- incubator/roller/trunk/src/org/apache/roller/business/runnable/ThreadManager.java (original)
+++ incubator/roller/trunk/src/org/apache/roller/business/runnable/ThreadManager.java Fri
Jan 12 08:58:10 2007
@@ -75,15 +75,6 @@
     
     
     /**
-     * Is a task currently locked?
-     * 
-     * @param task The RollerTask to check the lock state for.
-     * @return boolean True if task is locked, False otherwise.
-     */
-    public boolean isLocked(RollerTask task);
-    
-    
-    /**
      * Shutdown.
      */
     public void shutdown();

Modified: incubator/roller/trunk/src/org/apache/roller/business/runnable/TurnoverReferersTask.java
URL: http://svn.apache.org/viewvc/incubator/roller/trunk/src/org/apache/roller/business/runnable/TurnoverReferersTask.java?view=diff&rev=495640&r1=495639&r2=495640
==============================================================================
--- incubator/roller/trunk/src/org/apache/roller/business/runnable/TurnoverReferersTask.java
(original)
+++ incubator/roller/trunk/src/org/apache/roller/business/runnable/TurnoverReferersTask.java
Fri Jan 12 08:58:10 2007
@@ -34,6 +34,10 @@
     
     private static Log log = LogFactory.getLog(TurnoverReferersTask.class);
     
+    // a unique id for this specific task instance
+    // this is meant to be unique for each client in a clustered environment
+    private String clientId = null;
+    
     // a String description of when to start this task
     private String startTimeDesc = "startOfDay";
     
@@ -48,6 +52,10 @@
         return "TurnoverReferersTask";
     }
     
+    public String getClientId() {
+        return clientId;
+    }
+    
     public Date getStartTime(Date currentTime) {
         return getAdjustedTime(currentTime, startTimeDesc);
     }
@@ -65,6 +73,12 @@
         
         // get relevant props
         Properties props = this.getTaskProperties();
+        
+        // extract clientId
+        String client = props.getProperty("clientId");
+        if(client != null) {
+            this.clientId = client;
+        }
         
         // extract start time
         String startTimeStr = props.getProperty("startTime");

Modified: incubator/roller/trunk/src/org/apache/roller/planet/tasks/RefreshEntriesTask.java
URL: http://svn.apache.org/viewvc/incubator/roller/trunk/src/org/apache/roller/planet/tasks/RefreshEntriesTask.java?view=diff&rev=495640&r1=495639&r2=495640
==============================================================================
--- incubator/roller/trunk/src/org/apache/roller/planet/tasks/RefreshEntriesTask.java (original)
+++ incubator/roller/trunk/src/org/apache/roller/planet/tasks/RefreshEntriesTask.java Fri
Jan 12 08:58:10 2007
@@ -35,6 +35,10 @@
     
     private static Log log = LogFactory.getLog(RefreshEntriesTask.class);
     
+    // a unique id for this specific task instance
+    // this is meant to be unique for each client in a clustered environment
+    private String clientId = null;
+    
     // a String description of when to start this task
     private String startTimeDesc = "startOfHour";
     
@@ -49,6 +53,10 @@
         return "RefreshEntriesTask";
     }
     
+    public String getClientId() {
+        return clientId;
+    }
+    
     public Date getStartTime(Date currentTime) {
         return getAdjustedTime(currentTime, startTimeDesc);
     }
@@ -66,6 +74,12 @@
         
         // get relevant props
         Properties props = this.getTaskProperties();
+        
+        // extract clientId
+        String client = props.getProperty("clientId");
+        if(client != null) {
+            this.clientId = client;
+        }
         
         // extract start time
         String startTimeStr = props.getProperty("startTime");

Modified: incubator/roller/trunk/src/org/apache/roller/planet/tasks/SyncWebsitesTask.java
URL: http://svn.apache.org/viewvc/incubator/roller/trunk/src/org/apache/roller/planet/tasks/SyncWebsitesTask.java?view=diff&rev=495640&r1=495639&r2=495640
==============================================================================
--- incubator/roller/trunk/src/org/apache/roller/planet/tasks/SyncWebsitesTask.java (original)
+++ incubator/roller/trunk/src/org/apache/roller/planet/tasks/SyncWebsitesTask.java Fri Jan
12 08:58:10 2007
@@ -47,6 +47,10 @@
     
     private static Log log = LogFactory.getLog(SyncWebsitesTask.class);
     
+    // a unique id for this specific task instance
+    // this is meant to be unique for each client in a clustered environment
+    private String clientId = "unspecifiedClientId";
+    
     // a String description of when to start this task
     private String startTimeDesc = "startOfDay";
     
@@ -61,6 +65,10 @@
         return "SyncWebsitesTask";
     }
     
+    public String getClientId() {
+        return clientId;
+    }
+    
     public Date getStartTime(Date currentTime) {
         return getAdjustedTime(currentTime, startTimeDesc);
     }
@@ -78,6 +86,12 @@
         
         // get relevant props
         Properties props = this.getTaskProperties();
+        
+        // extract clientId
+        String client = props.getProperty("clientId");
+        if(client != null) {
+            this.clientId = client;
+        }
         
         // extract start time
         String startTimeStr = props.getProperty("startTime");

Modified: incubator/roller/trunk/tests/org/apache/roller/business/TaskLockTest.java
URL: http://svn.apache.org/viewvc/incubator/roller/trunk/tests/org/apache/roller/business/TaskLockTest.java?view=diff&rev=495640&r1=495639&r2=495640
==============================================================================
--- incubator/roller/trunk/tests/org/apache/roller/business/TaskLockTest.java (original)
+++ incubator/roller/trunk/tests/org/apache/roller/business/TaskLockTest.java Fri Jan 12 08:58:10
2007
@@ -66,23 +66,15 @@
         RollerTask task = new TestTask();
         
         // try to acquire a lock
-        boolean lockAcquired = mgr.acquireLock(task);
-        assertTrue(lockAcquired);
+        assertTrue(mgr.acquireLock(task));
         TestUtils.endSession(true);
         
         // make sure task is locked
-        boolean stillLocked = mgr.isLocked(task);
-        assertTrue(stillLocked);
+        assertFalse(mgr.acquireLock(task));
         TestUtils.endSession(true);
         
         // try to release a lock
-        boolean lockReleased = mgr.releaseLock(task);
-        assertTrue(lockReleased);
-        TestUtils.endSession(true);
-        
-        // make sure task is not locked
-        stillLocked = mgr.isLocked(task);
-        assertFalse(stillLocked);
+        assertTrue(mgr.releaseLock(task));
         TestUtils.endSession(true);
     }
     
@@ -90,6 +82,7 @@
     class TestTask extends RollerTask {
         
         public String getName() { return "TestTask"; }
+        public String getClientId() { return "TestTaskClientId"; }
         public Date getStartTime(Date current) { return current; }
         public int getLeaseTime() { return 300; }
         public int getInterval() { return 1800; }

Modified: incubator/roller/trunk/web/WEB-INF/classes/roller.properties
URL: http://svn.apache.org/viewvc/incubator/roller/trunk/web/WEB-INF/classes/roller.properties?view=diff&rev=495640&r1=495639&r2=495640
==============================================================================
--- incubator/roller/trunk/web/WEB-INF/classes/roller.properties (original)
+++ incubator/roller/trunk/web/WEB-INF/classes/roller.properties Fri Jan 12 08:58:10 2007
@@ -269,6 +269,9 @@
 # Tasks which are enabled.  Only tasks listed here will be run.
 tasks.enabled=ResetHitCountsTask,TurnoverReferersTask,PingQueueTask
 
+# client identifier.  should be unique for each instance in a cluster.
+tasks.clientId=defaultClientId
+
 # Reset hit counts
 tasks.ResetHitCountsTask.class=org.apache.roller.business.runnable.ResetHitCountsTask
 tasks.ResetHitCountsTask.startTime=startOfDay



Mime
View raw message