From bebd9317775818bcbaf50ba66f1e2116491c1018 Mon Sep 17 00:00:00 2001
From: Axel Uhl <axel.uhl@sap.com>
Date: Tue, 28 Feb 2012 17:42:37 +0100
Subject: [PATCH] fixing bug 340 by fetching all data required while still
 owning the lock on the GPS fix track

---
 .../sailing/domain/tracking/TrackedRace.java  |  7 +++++
 .../impl/DynamicGPSFixMovingTrackImpl.java    | 30 +++++++++++++------
 .../tracking/impl/RaceRankComparator.java     |  1 +
 .../domain/tracking/impl/TrackImpl.java       | 12 ++++++--
 4 files changed, 39 insertions(+), 11 deletions(-)

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 d804aef56c4..2bb6505815f 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
@@ -194,6 +194,13 @@ public interface TrackedRace {
      */
     TimePoint getTimePointOfNewestEvent();
 
+    /**
+     * @return the mark passings for <code>competitor</code> in this race received so far; the mark passing objects are
+     * returned such that their {@link MarkPassing#getWaypoint() waypoints} are ordered in the same way they are ordered
+     * in the race's {@link Course}. Note, that this doesn't necessarily guarantee ascending time points, particularly
+     * if premature mark passings have been detected accidentally as can be the case with some tracking providers such
+     * as TracTrac.
+     */
     NavigableSet<MarkPassing> getMarkPassings(Competitor competitor);
 
     void removeWind(Wind wind, WindSource windSource);
diff --git a/java/com.sap.sailing.domain/src/com/sap/sailing/domain/tracking/impl/DynamicGPSFixMovingTrackImpl.java b/java/com.sap.sailing.domain/src/com/sap/sailing/domain/tracking/impl/DynamicGPSFixMovingTrackImpl.java
index 18dd813b8a0..37f384df959 100755
--- a/java/com.sap.sailing.domain/src/com/sap/sailing/domain/tracking/impl/DynamicGPSFixMovingTrackImpl.java
+++ b/java/com.sap.sailing.domain/src/com/sap/sailing/domain/tracking/impl/DynamicGPSFixMovingTrackImpl.java
@@ -100,10 +100,15 @@ public class DynamicGPSFixMovingTrackImpl<ItemType> extends DynamicTrackImpl<Ite
             NavigableSet<GPSFixMoving> fixesToUseForSpeedEstimation) {
         // TODO factor out the obtaining of relevant fixes which should be the same in super.getEstimatedSpeed(at)
         DummyGPSFixMoving atTimed = new DummyGPSFixMoving(at);
-        NavigableSet<GPSFixMoving> beforeSet = fixesToUseForSpeedEstimation.headSet(atTimed, /* inclusive */ false);
-        NavigableSet<GPSFixMoving> afterSet = fixesToUseForSpeedEstimation.tailSet(atTimed, /* inclusive */ true);
         List<GPSFixMoving> relevantFixes = new LinkedList<GPSFixMoving>();
+        boolean beforeSetEmpty;
+        GPSFixMoving beforeSetLast = null;
         synchronized (this) {
+            NavigableSet<GPSFixMoving> beforeSet = fixesToUseForSpeedEstimation.headSet(atTimed, /* inclusive */ false);
+            beforeSetEmpty = beforeSet.isEmpty(); // ask this while holding the lock
+            if (!beforeSetEmpty) {
+                beforeSetLast = beforeSet.last();
+            }
             for (GPSFixMoving beforeFix : beforeSet.descendingSet()) {
                 if (at.asMillis() - beforeFix.getTimePoint().asMillis() > getMillisecondsOverWhichToAverage() / 2) {
                     break;
@@ -111,7 +116,14 @@ public class DynamicGPSFixMovingTrackImpl<ItemType> extends DynamicTrackImpl<Ite
                 relevantFixes.add(0, beforeFix);
             }
         }
+        boolean afterSetEmpty;
+        GPSFixMoving afterSetFirst = null;
         synchronized (this) {
+            NavigableSet<GPSFixMoving> afterSet = fixesToUseForSpeedEstimation.tailSet(atTimed, /* inclusive */ true);
+            afterSetEmpty = afterSet.isEmpty(); // ask this while holding the lock
+            if (!afterSetEmpty) {
+                afterSetFirst = afterSet.first();
+            }
             for (GPSFixMoving afterFix : afterSet) {
                 if (afterFix.getTimePoint().asMillis() - at.asMillis() > getMillisecondsOverWhichToAverage() / 2) {
                     break;
@@ -121,16 +133,16 @@ public class DynamicGPSFixMovingTrackImpl<ItemType> extends DynamicTrackImpl<Ite
         }
         if (relevantFixes.isEmpty()) {
             // find the fix closest to "at":
-            if (beforeSet.isEmpty()) {
-                if (!afterSet.isEmpty()) {
-                    relevantFixes.add(afterSet.first());
+            if (beforeSetEmpty) {
+                if (!afterSetEmpty) {
+                    relevantFixes.add(afterSetFirst);
                 }
             } else {
-                if (afterSet.isEmpty()) {
-                    relevantFixes.add(beforeSet.last());
+                if (afterSetEmpty) {
+                    relevantFixes.add(beforeSetLast);
                 } else {
-                    GPSFixMoving beforeFix = beforeSet.last();
-                    GPSFixMoving afterFix = afterSet.first();
+                    GPSFixMoving beforeFix = beforeSetLast;
+                    GPSFixMoving afterFix = afterSetFirst;
                     relevantFixes.add(at.asMillis() - beforeFix.getTimePoint().asMillis() <= afterFix.getTimePoint()
                             .asMillis() - at.asMillis() ? beforeFix : afterFix);
                 }
diff --git a/java/com.sap.sailing.domain/src/com/sap/sailing/domain/tracking/impl/RaceRankComparator.java b/java/com.sap.sailing.domain/src/com/sap/sailing/domain/tracking/impl/RaceRankComparator.java
index 5e0431f2ffc..602e3dbdf61 100755
--- a/java/com.sap.sailing.domain/src/com/sap/sailing/domain/tracking/impl/RaceRankComparator.java
+++ b/java/com.sap.sailing.domain/src/com/sap/sailing/domain/tracking/impl/RaceRankComparator.java
@@ -43,6 +43,7 @@ public class RaceRankComparator implements Comparator<Competitor> {
         if (o1 == o2) {
             result = 0;
         } else {
+            // TODO see also bug 340/342; need to synchronize on TrackedRace to avoid concurrent updates to MarkPassings, although this is course-grained
             NavigableSet<MarkPassing> o1MarkPassings = trackedRace.getMarkPassings(o1).headSet(
                     markPassingWithTimePoint, /* inclusive */true);
             NavigableSet<MarkPassing> o2MarkPassings = trackedRace.getMarkPassings(o2).headSet(
diff --git a/java/com.sap.sailing.domain/src/com/sap/sailing/domain/tracking/impl/TrackImpl.java b/java/com.sap.sailing.domain/src/com/sap/sailing/domain/tracking/impl/TrackImpl.java
index 14e40d57bec..1c068b93054 100755
--- a/java/com.sap.sailing.domain/src/com/sap/sailing/domain/tracking/impl/TrackImpl.java
+++ b/java/com.sap.sailing.domain/src/com/sap/sailing/domain/tracking/impl/TrackImpl.java
@@ -1,5 +1,6 @@
 package com.sap.sailing.domain.tracking.impl;
 
+import java.util.ConcurrentModificationException;
 import java.util.Iterator;
 import java.util.NavigableSet;
 
@@ -39,6 +40,10 @@ public abstract class TrackImpl<FixType extends Timed> implements Track<FixType>
         this.fixes = fixes;
     }
 
+    /**
+     * Callers that want to iterate over the collection returned need to synchronize on <code>this</code> object to avoid
+     * {@link ConcurrentModificationException}s.
+     */
     protected NavigableSet<FixType> getInternalRawFixes() {
         @SuppressWarnings("unchecked")
         NavigableSet<FixType> result = (NavigableSet<FixType>) fixes;
@@ -46,8 +51,11 @@ public abstract class TrackImpl<FixType extends Timed> implements Track<FixType>
     }
     
     /**
-     * @return the smoothened fixes; this implementation simply delegates to {@link #getInternalRawFixes()} because for only
-     *         {@link Timed} fixes we can't know how to remove outliers. Subclasses that constrain the
+     * Callers that want to iterate over the collection returned need to synchronize on <code>this</code> object to
+     * avoid {@link ConcurrentModificationException}s.
+     * 
+     * @return the smoothened fixes; this implementation simply delegates to {@link #getInternalRawFixes()} because for
+     *         only {@link Timed} fixes we can't know how to remove outliers. Subclasses that constrain the
      *         <code>FixType</code> may provide smoothening implementations.
      */
     protected NavigableSet<FixType> getInternalFixes() {