Sourceforge 1986987 Deadlock between ComThread and ROT. Also added thread name to Jacob log output.

Author: clay_shooter

jacob/docs/ReleaseNotes.html

@@ -10,7 +10,12 @@ <h3>Tracked Changes</h3>
   <tr> 
     <td width="13%" valign="top">2011706</td>
     <td width="87%" valign="top">Fixed windows memory corruption unhooking 
-    call back proxies</td>
+    call back proxy</td>
+  </tr>
+  <tr> 
+    <td width="13%" valign="top">1986987 </td>
+    <td width="87%" valign="top">Possible deadlock when multiple threads starting 
+    and stopping that rely on implicit ComThread.InitMTA</td>
   </tr>
   <tr> 
     <td width="13%" valign="top">&nbsp;</td>
@@ -148,6 +153,7 @@ <h3>Tracked Changes</h3>
     <td width="87%" valign="top">(M7) Jacob DLL name can now be customized to 
     support bundling of Jacob in other products.</td> 
   </tr>
+  <tr>
     <td width="13%" valign="top">1845039 </td>
     <td width="87%" valign="top">(M6) Jacob DLL names are now qualified by platform and 
     release.  The JacobLibraryLoader now determines the correct 32bit or 64bit 

jacob/src/com/jacob/com/JacobObject.java

@@ -96,8 +96,8 @@ public static String getBuildVersion() {
 	 */
 	protected static void debug(String istrMessage) {
 		if (isDebugEnabled()) {
-			System.out.println(istrMessage + " in thread "
-					+ Thread.currentThread().getName());
+			System.out.println(Thread.currentThread().getName() + ": "
+					+ istrMessage);
 		}
 	}
 

jacob/src/com/jacob/com/ROT.java

@@ -95,7 +95,7 @@ protected synchronized static Map<JacobObject, String> addThread() {
 	}
 
 	/**
-	 * returns the pool for this thread if it exists. can create a new one if
+	 * Returns the pool for this thread if it exists. can create a new one if
 	 * you wish by passing in TRUE
 	 * 
 	 * @param createIfDoesNotExist
@@ -115,19 +115,20 @@ protected synchronized static Map<JacobObject, String> getThreadObjects(
 	 * Iterates across all of the entries in the Hashmap in the rot that
 	 * corresponds to this thread. This calls safeRelease() on each entry and
 	 * then clears the map when done and removes it from the rot. All traces of
-	 * this thread's objects will disapear. This is called by COMThread in the
+	 * this thread's objects will disappear. This is called by COMThread in the
 	 * tear down and provides a synchronous way of releasing memory
 	 */
-	protected synchronized static void clearObjects() {
-		Map<JacobObject, String> tab = getThreadObjects(false);
+	protected static void clearObjects() {
 		if (JacobObject.isDebugEnabled()) {
 			JacobObject.debug("ROT: " + rot.keySet().size()
 					+ " thread tables exist");
 		}
+
+		Map<JacobObject, String> tab = getThreadObjects(false);
 		if (tab != null) {
 			if (JacobObject.isDebugEnabled()) {
 				JacobObject.debug("ROT: " + tab.keySet().size()
-						+ " objects to clear in this thread ");
+						+ " objects to clear in this thread's ROT ");
 			}
 			// walk the values
 			Iterator<JacobObject> it = tab.keySet().iterator();
@@ -155,13 +156,11 @@ protected synchronized static void clearObjects() {
 					}
 					o.safeRelease();
 				}
-				// used to be an iterator.remove() but why bother when we're
-				// nuking them all anyway?
 			}
 			// empty the collection
 			tab.clear();
 			// remove the collection from rot
-			rot.remove(Thread.currentThread().getName());
+			ROT.removeThread();
 			if (JacobObject.isDebugEnabled()) {
 				JacobObject.debug("ROT: thread table cleared and removed");
 			}
@@ -172,31 +171,52 @@ protected synchronized static void clearObjects() {
 		}
 	}
 
+	/**
+	 * Removes the map from the rot that is associated with the current thread.
+	 */
+	private synchronized static void removeThread() {
+		// should this see if it exists first?
+		rot.remove(Thread.currentThread().getName());
+	}
+
 	/**
 	 * @deprecated the java model leave the responsibility of clearing up
 	 *             objects to the Garbage Collector. Our programming model
 	 *             should not require that the user specifically remove object
-	 *             from the thread.
-	 * 
-	 * This will remove an object from the ROT
+	 *             from the thread. <br>
+	 *             This will remove an object from the ROT <br>
+	 *             This does not need to be synchronized because only the rot
+	 *             modification related methods need to synchronized. Each
+	 *             individual map is only modified in a single thread.
 	 * @param o
 	 */
 	@Deprecated
-	protected synchronized static void removeObject(JacobObject o) {
-		String t_name = Thread.currentThread().getName();
-		Map<JacobObject, String> tab = rot.get(t_name);
+	protected static void removeObject(JacobObject o) {
+		Map<JacobObject, String> tab = ROT.getThreadObjects(false);
 		if (tab != null) {
 			tab.remove(o);
 		}
 		o.safeRelease();
 	}
 
 	/**
-	 * adds an object to the HashMap for the current thread
+	 * Adds an object to the HashMap for the current thread. <br>
+	 * <p>
+	 * This method does not need to be threaded because the only concurrent
+	 * modification risk is on the hash map that contains all of the thread
+	 * related hash maps. The individual thread related maps are only used on a
+	 * per thread basis so there isn't a locking issue.
+	 * <p>
+	 * In addition, this method cannot be threaded because it calls
+	 * ComThread.InitMTA. The ComThread object has some methods that call ROT so
+	 * we could end up deadlocked. This method should be safe without the
+	 * synchronization because the ROT works on per thread basis and the methods
+	 * that add threads and remove thread related entries are all synchronized
+	 * 
 	 * 
 	 * @param o
 	 */
-	protected synchronized static void addObject(JacobObject o) {
+	protected static void addObject(JacobObject o) {
 		// check the system property to see if this class is put in the ROT
 		// the default value is "true" which simulates the old behavior
 		String shouldIncludeClassInROT = System.getProperty(o.getClass()
@@ -208,18 +228,22 @@ protected synchronized static void addObject(JacobObject o) {
 						+ o.getClass().getName() + " not added to ROT");
 			}
 		} else {
+			// first see if we have a table for this thread
 			Map<JacobObject, String> tab = getThreadObjects(false);
 			if (tab == null) {
 				// this thread has not been initialized as a COM thread
 				// so make it part of MTA for backwards compatibility
 				ComThread.InitMTA(false);
+				// don't really need the "true" because the InitMTA will have
+				// called back to the ROT to create a table for this thread
 				tab = getThreadObjects(true);
 			}
 			if (JacobObject.isDebugEnabled()) {
 				JacobObject.debug("ROT: adding " + o + "->"
 						+ o.getClass().getName()
 						+ " table size prior to addition:" + tab.size());
 			}
+			// add the object to the table that is specific to this thread
 			if (tab != null) {
 				tab.put(o, null);
 			}

jacob/unittest/com/jacob/com/JacobDeadlockTest.java

@@ -0,0 +1,120 @@
+package com.jacob.com;
+
+import com.jacob.test.BaseTestCase;
+
+/**
+ * Sourceforge defect report 1986987 July 2008. This test case demonstrated a
+ * deadlock issue.
+ * <UL>
+ * <LI>One thread attempts to create an object in a thread where InitMTA has
+ * not been called. This results in ROT.addObject being called which then calls
+ * ComThread.InitMTA
+ * <LI>One thread attempts to call ComThread.release() which then calls ROT
+ * .clear objects.
+ * </UL>
+ * The result is one thread with a call sequence ComThread--ROT and the other
+ * with a sequence ROT--ComThread resulting in deadlock.
+ * <p>
+ * This test will fail with debug logging turned on because of the amount of
+ * time it takes to write the debug output.
+ * 
+ * @author jsamarziya
+ * 
+ */
+public class JacobDeadlockTest extends BaseTestCase {
+	private static final long TIMEOUT = 5000l;
+
+	/** Thread component */
+	public static class TestThread extends Thread {
+		private final int id;
+		private final boolean initCOM;
+		private final boolean writeOutput;
+
+		/**
+		 * constructor for ThestThread
+		 * 
+		 * @param id
+		 * @param initCOM
+		 * @param writeOutput
+		 * 
+		 */
+		public TestThread(int id, boolean initCOM, boolean writeOutput) {
+			this.id = id;
+			this.initCOM = initCOM;
+			this.writeOutput = writeOutput;
+		}
+
+		@Override
+		public void run() {
+			for (int i = 0; i < 1000; i++) {
+				log("iteration " + i);
+				if (initCOM) {
+					log("Initializing COM thread");
+					ComThread.InitMTA(false);
+				}
+				log("Creating JacobObject");
+				new JacobObject();
+				log("Releasing COM thread");
+				ComThread.Release();
+			}
+			log("Exiting Java Thread");
+		}
+
+		private void log(String message) {
+			if (writeOutput) {
+				System.out.println(Thread.currentThread().getName()
+						+ ": TestThread[" + id + "] " + " " + " - " + message);
+			}
+		}
+	}
+
+	/**
+	 * This test shows that if ComThread.Init() is called explicitly, no problem
+	 * occurs.
+	 * 
+	 * @throws InterruptedException
+	 */
+	public void testShowNoProblemIfCOMIsInitialized()
+			throws InterruptedException {
+		runTest(2, true, false);
+		runTest(100, true, false);
+	}
+
+	/**
+	 * This test shows that if only one thread is creating COM objects, no
+	 * problem occurs.
+	 * 
+	 * @throws InterruptedException
+	 */
+	public void testShowNoProblemIfSingleThreaded() throws InterruptedException {
+		runTest(1, false, false);
+		runTest(1, true, false);
+	}
+
+	/**
+	 * Runs the test with two threads, which don't initialize the COM thread.
+	 * 
+	 * This test will always fail.
+	 * 
+	 * @throws InterruptedException
+	 */
+	public void testShowDeadlockProblem() throws InterruptedException {
+		runTest(2, false, true);
+	}
+
+	private void runTest(int numberOfThreads, boolean initCOM,
+			boolean writeOutput) throws InterruptedException {
+		Thread[] threads = new Thread[numberOfThreads];
+		for (int i = 0; i < threads.length; i++) {
+			threads[i] = new TestThread(i, initCOM, writeOutput);
+			threads[i].start();
+		}
+		for (int i = 0; i < threads.length; i++) {
+			threads[i].join(TIMEOUT);
+			if (threads[i].isAlive()) {
+				fail("thread " + i + " failed to finish in " + TIMEOUT
+						+ " milliseconds");
+			}
+		}
+	}
+}