From aa1627e129d358432447bb0c6bbec2c69d4f7fd2 Mon Sep 17 00:00:00 2001
From: Axel Uhl <axel.uhl@sap.com>
Date: Tue, 21 Jun 2016 00:10:08 +0200
Subject: [PATCH] fixing bug 3728 (deadlock) by copying mark passing for
 waypoint under the lock, then releasing, then calling into deep stacks

Change-Id: Idc28d6c79781e9a174fce8ee8646f64b08b0a11c
---
 .../domain/ranking/AbstractRankingMetric.java | 41 ++++++++++++-------
 .../sailing/domain/tracking/TrackedRace.java  |  4 ++
 2 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/java/com.sap.sailing.domain/src/com/sap/sailing/domain/ranking/AbstractRankingMetric.java b/java/com.sap.sailing.domain/src/com/sap/sailing/domain/ranking/AbstractRankingMetric.java
index ad058e1738c..92d4e1494e4 100755
--- a/java/com.sap.sailing.domain/src/com/sap/sailing/domain/ranking/AbstractRankingMetric.java
+++ b/java/com.sap.sailing.domain/src/com/sap/sailing/domain/ranking/AbstractRankingMetric.java
@@ -1,8 +1,10 @@
 package com.sap.sailing.domain.ranking;
 
+import java.util.ArrayList;
 import java.util.Comparator;
 import java.util.HashMap;
 import java.util.Iterator;
+import java.util.List;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
@@ -29,6 +31,7 @@ import com.sap.sailing.domain.tracking.WindPositionMode;
 import com.sap.sailing.domain.tracking.impl.AbstractRaceRankComparator;
 import com.sap.sse.common.Duration;
 import com.sap.sse.common.TimePoint;
+import com.sap.sse.common.Util;
 
 public abstract class AbstractRankingMetric implements RankingMetric {
     private static final long serialVersionUID = -3671039530564696392L;
@@ -664,24 +667,32 @@ public abstract class AbstractRankingMetric implements RankingMetric {
         if (firstAroundMark != null) {
             result = firstAroundMark;
         } else {
-            Iterable<MarkPassing> markPassingsForLegStart = getTrackedRace().getMarkPassingsInOrder(trackedLeg.getLeg().getFrom());
+            List<MarkPassing> copyOfMarkPassingsForLegStart = new ArrayList<>();
+            {   // scope the markPassingsForLegStart, so it is no longer used outside this block
+                final Iterable<MarkPassing> markPassingsForLegStart = getTrackedRace().getMarkPassingsInOrder(trackedLeg.getLeg().getFrom());
+                // See bug 3728: obtaining lock for mark passings in order for a waypoint before code potentially called from here,
+                // e.g., through getWindwardDistanceTraveled(...), tries to obtain the read lock for mark passings for a competitor can
+                // result in a deadlock. Therefore, we copy the mark passings for the leg start under the lock, then release it again
+                // before calling into a deep stack with getWindwardDistanceTraveled(...) which may well obtain a read lock on
+                // the mark passings for a competitor.
+                getTrackedRace().lockForRead(markPassingsForLegStart);
+                try {
+                    Util.addAll(markPassingsForLegStart, copyOfMarkPassingsForLegStart);
+                } finally {
+                    getTrackedRace().unlockAfterRead(markPassingsForLegStart);
+                }
+            }
             Distance maxWindwardDistanceTraveled = new MeterDistance(Double.MIN_VALUE);
             Competitor competitorFarthestAlong = null;
-            getTrackedRace().lockForRead(markPassingsForLegStart);
-            try {
-                for (MarkPassing mp : markPassingsForLegStart) {
-                    if (mp.getTimePoint().after(timePoint)) {
-                        break;
-                    }
-                    final Distance windwardDistanceTraveled = getWindwardDistanceTraveled(mp.getCompetitor(), mp.getWaypoint(),
-                            timePoint, cache);
-                    if (windwardDistanceTraveled.compareTo(maxWindwardDistanceTraveled) > 0) {
-                        maxWindwardDistanceTraveled = windwardDistanceTraveled;
-                        competitorFarthestAlong = mp.getCompetitor();
-                    }
+            for (MarkPassing mp : copyOfMarkPassingsForLegStart) {
+                if (mp.getTimePoint().after(timePoint)) {
+                    break;
+                }
+                final Distance windwardDistanceTraveled = getWindwardDistanceTraveled(mp.getCompetitor(), mp.getWaypoint(), timePoint, cache);
+                if (windwardDistanceTraveled.compareTo(maxWindwardDistanceTraveled) > 0) {
+                    maxWindwardDistanceTraveled = windwardDistanceTraveled;
+                    competitorFarthestAlong = mp.getCompetitor();
                 }
-            } finally {
-                getTrackedRace().unlockAfterRead(markPassingsForLegStart);
             }
             result = competitorFarthestAlong;
         }
diff --git a/java/com.sap.sailing.domain/src/com/sap/sailing/domain/tracking/TrackedRace.java b/java/com.sap.sailing.domain/src/com/sap/sailing/domain/tracking/TrackedRace.java
index 1d7d18451ab..98fd18ed4bb 100755
--- a/java/com.sap.sailing.domain/src/com/sap/sailing/domain/tracking/TrackedRace.java
+++ b/java/com.sap.sailing.domain/src/com/sap/sailing/domain/tracking/TrackedRace.java
@@ -414,6 +414,10 @@ public interface TrackedRace extends Serializable, IsManagedByCache<SharedDomain
      * first obtains the mark passings read lock and later, while holding on to that lock, asks for the course's read
      * lock, a deadlock may result.<p>
      * 
+     * Furthermore, when trying to acquire both, a lock for the {@link #getMarkPassings(Competitor) mark passings for a competitor}
+     * and a lock for the {@link #getMarkPassingsInOrder(Waypoint) mark passings for a waypoint, this needs to happen in exactly
+     * this order, or a deadlock may result.<p>
+     * 
      * The {@link #unlockAfterRead(Iterable)} method will symmetrically unlock the course's read lock after releasing the
      * read lock for the mark passings.
      */