From 9eef604f2bc1564427691faf002ae00a4b26eff1 Mon Sep 17 00:00:00 2001
From: Kelven Yang <kelven@cloud.com>
Date: Mon, 3 Jan 2011 09:23:42 -0800
Subject: [PATCH] bug 7685: a race condition caused DB connection from the pool
 to be left alone which can trigger mysql driver NPE exception

---
 utils/src/com/cloud/utils/db/DbUtil.java     | 60 +++++++++-----------
 utils/src/com/cloud/utils/db/GlobalLock.java | 34 +++++------
 2 files changed, 45 insertions(+), 49 deletions(-)

diff --git a/utils/src/com/cloud/utils/db/DbUtil.java b/utils/src/com/cloud/utils/db/DbUtil.java
index abeb8cc0a22..a7dc8f4c540 100755
--- a/utils/src/com/cloud/utils/db/DbUtil.java
+++ b/utils/src/com/cloud/utils/db/DbUtil.java
@@ -49,39 +49,33 @@ public class DbUtil {
     private static Map<String, Connection> s_connectionForGlobalLocks = new HashMap<String, Connection>();
     
     public static Connection getConnectionForGlobalLocks(String name, boolean forLock) {
-    	while(true) {
-	    	synchronized(s_connectionForGlobalLocks) {
-	    		if(forLock) {
-	    			if(s_connectionForGlobalLocks.get(name) != null) {
-	    				s_logger.error("Sanity check failed, global lock name " + name + " is in already in use");
-	    			}
-	    			
-	    			Connection connection = Transaction.getStandaloneConnection();
-	    			if(connection != null) {
-	    				try {
-							connection.setAutoCommit(true);
-						} catch (SQLException e) {
-							try {
-								connection.close();
-							} catch(SQLException sqlException) {
-							}
-							return null;
+    	synchronized(s_connectionForGlobalLocks) {
+    		if(forLock) {
+    			if(s_connectionForGlobalLocks.get(name) != null) {
+    				s_logger.error("Sanity check failed, global lock name " + name + " is already in use");
+    				assert(false);
+    			}
+    			
+    			Connection connection = Transaction.getStandaloneConnection();
+    			if(connection != null) {
+    				try {
+						connection.setAutoCommit(true);
+					} catch (SQLException e) {
+						try {
+							connection.close();
+						} catch(SQLException sqlException) {
 						}
-						s_connectionForGlobalLocks.put(name, connection);
-						return connection;
-	    			}
-	    		} else {
-	    			Connection connection = s_connectionForGlobalLocks.get(name);
-	    			s_connectionForGlobalLocks.remove(name);
-	    			return connection;
-	    		}
-	    	}
-	    	
-			s_logger.warn("Unable to acquire dabase connection for global lock " + name + ", waiting for someone to release and retrying...");
-	    	try {
-				Thread.sleep(1000);
-			} catch (InterruptedException e) {
-			}
+						return null;
+					}
+					s_connectionForGlobalLocks.put(name, connection);
+					return connection;
+    			}
+    	    	return null;
+    		} else {
+    			Connection connection = s_connectionForGlobalLocks.get(name);
+    			s_connectionForGlobalLocks.remove(name);
+    			return connection;
+    		}
     	}
     }
     
@@ -237,7 +231,7 @@ public class DbUtil {
         	if (pstmt != null) {
         		try {
         			pstmt.close();
-        		} catch (SQLException e) {
+        		} catch (Throwable e) {
         			s_logger.error("What the heck? ", e);
         		}
         	}
diff --git a/utils/src/com/cloud/utils/db/GlobalLock.java b/utils/src/com/cloud/utils/db/GlobalLock.java
index 356c63d97e5..789da2deb25 100644
--- a/utils/src/com/cloud/utils/db/GlobalLock.java
+++ b/utils/src/com/cloud/utils/db/GlobalLock.java
@@ -45,9 +45,9 @@ public class GlobalLock {
     protected final static Logger s_logger = Logger.getLogger(GlobalLock.class);
 
 	private String name;
-	private volatile int lockCount = 0;
+	private int lockCount = 0;
 	private Thread ownerThread = null;
-	
+	
 	private int referenceCount = 0;
 	private long holdingStartTick = 0;
 	
@@ -65,21 +65,21 @@ public class GlobalLock {
 	}
 	
 	public int releaseRef() {
-		boolean releaseInternLock = false;
-		int refCount;
-		synchronized(this) {
-			referenceCount--;
-			refCount = referenceCount;
-			
-			if(referenceCount < 0)
-				s_logger.warn("Unmatched Global lock " + name + " reference usage detected, check your code!");
-			
-			if(referenceCount == 0)
-				releaseInternLock = true;
+		int refCount;
+		
+		synchronized(s_lockMap) {	// // lock in sequence to prevent deadlock
+			synchronized(this) {
+				referenceCount--;
+				refCount = referenceCount;
+				
+				if(referenceCount < 0)
+					s_logger.warn("Unmatched Global lock " + name + " reference usage detected, check your code!");
+				
+				if(referenceCount == 0)
+					releaseInternLock(name);
+			}
 		}
 		
-		if(releaseInternLock)
-			releaseInternLock(name);
 		return refCount;
 	}
 
@@ -137,9 +137,9 @@ public class GlobalLock {
 						continue;
 					} else {
 						// we will discount the time that has been spent in previous waiting
+						ownerThread = Thread.currentThread();
 						if(DbUtil.getGlobalLock(name, remainingMilliSeconds / 1000)) {
 							lockCount++;
-							ownerThread = Thread.currentThread();
 							holdingStartTick = System.currentTimeMillis();
 							
 							// keep the lock in the intern map when we got the lock from database
@@ -148,6 +148,8 @@ public class GlobalLock {
 							if(s_logger.isTraceEnabled())
 								s_logger.trace("lock " + name + " is acquired, lock count :" + lockCount);
 							return true;
+						} else {
+							ownerThread = null;
 						}
 						return false;
 					}