fixing bug 340 by fetching all data required while still owning the lock on the GPS fix track

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);

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);
                 }

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(

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() {