From 32a6d7000a4dab5f208c9d5e78f35b314edbaec3 Mon Sep 17 00:00:00 2001
From: David Roazen <droazen@broadinstitute.org>
Date: Wed, 24 Oct 2012 13:06:34 -0400
Subject: [PATCH] Thread-safe ReadGroupCovariate

The ReadGroupCovariate class was not thread-safe. This led to horrible race conditions
in multithreaded runs of the BQSR where (for example) the same read group could get
inserted into the reverse lookup table twice with different IDs.

Should fix the intermittent crash reported in GSA-492.
---
 .../covariates/ReadGroupCovariate.java        | 29 +++++++++++++++----
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/public/java/src/org/broadinstitute/sting/utils/recalibration/covariates/ReadGroupCovariate.java b/public/java/src/org/broadinstitute/sting/utils/recalibration/covariates/ReadGroupCovariate.java
index 85568dac9..29c15adf7 100755
--- a/public/java/src/org/broadinstitute/sting/utils/recalibration/covariates/ReadGroupCovariate.java
+++ b/public/java/src/org/broadinstitute/sting/utils/recalibration/covariates/ReadGroupCovariate.java
@@ -66,7 +66,9 @@ public class ReadGroupCovariate implements RequiredCovariate {
     }
 
     @Override
-    public String formatKey(final int key) {
+    public synchronized String formatKey(final int key) {
+        // This method is synchronized so that we don't attempt to do a get()
+        // from the reverse lookup table while that table is being updated
         return readGroupReverseLookupTable.get(key);
     }
 
@@ -76,17 +78,32 @@ public class ReadGroupCovariate implements RequiredCovariate {
     }
 
     private int keyForReadGroup(final String readGroupId) {
-        if (!readGroupLookupTable.containsKey(readGroupId)) {
-            readGroupLookupTable.put(readGroupId, nextId);
-            readGroupReverseLookupTable.put(nextId, readGroupId);
-            nextId++;
+        // Rather than synchronize this entire method (which would be VERY expensive for walkers like the BQSR),
+        // synchronize only the table updates.
+
+        // Before entering the synchronized block, check to see if this read group is not in our tables.
+        // If it's not, either we will have to insert it, OR another thread will insert it first.
+        // This preliminary check avoids doing any synchronization most of the time.
+        if ( ! readGroupLookupTable.containsKey(readGroupId) ) {
+
+            synchronized ( this ) {
+
+                // Now we need to make sure the key is STILL not there, since another thread may have come along
+                // and inserted it while we were waiting to enter this synchronized block!
+                if ( ! readGroupLookupTable.containsKey(readGroupId) ) {
+                    readGroupLookupTable.put(readGroupId, nextId);
+                    readGroupReverseLookupTable.put(nextId, readGroupId);
+                    nextId++;
+                }
+            }
         }
 
         return readGroupLookupTable.get(readGroupId);
     }
 
     @Override
-    public int maximumKeyValue() {
+    public synchronized int maximumKeyValue() {
+        // Synchronized so that we don't query table size while the tables are being updated
         return readGroupLookupTable.size() - 1;
     }