From 18b21a6a3ef0f583a4fa29bb25231b7e89def0d6 Mon Sep 17 00:00:00 2001 From: Dennis Guse Date: Sun, 3 Apr 2022 12:28:15 +0200 Subject: [PATCH] Bugfix: prevent race condition in TrackPointCreator. This race condition leads to a crash in TrackStatisticsUpdater as the time might appear to move backwards if the created TrackPoints are not stored in order. Might happen if multiple sensors (BLE or GPS) are used in simultaneously. Every method affected by a race condition is now synchronized. Problem was introduced in c4b8082efc87a4eb48bc39cb5c7cd91ba1ab10f6 Fixes #1151. --- .../io/file/importer/ExportImportTest.java | 2 +- .../services/handlers/GPSHandlerTest.java | 9 +++--- .../services/handlers/GPSHandler.java | 13 ++++----- .../services/handlers/TrackPointCreator.java | 29 ++++++++++++------- .../opentracks/util/LocationUtils.java | 9 ++++++ 5 files changed, 38 insertions(+), 24 deletions(-) diff --git a/src/androidTest/java/de/dennisguse/opentracks/io/file/importer/ExportImportTest.java b/src/androidTest/java/de/dennisguse/opentracks/io/file/importer/ExportImportTest.java index 05b6c277e..71862105d 100644 --- a/src/androidTest/java/de/dennisguse/opentracks/io/file/importer/ExportImportTest.java +++ b/src/androidTest/java/de/dennisguse/opentracks/io/file/importer/ExportImportTest.java @@ -469,7 +469,7 @@ public class ExportImportTest { return sensorDataSet; } }); - trackPointCreator.onChange(null); + trackPointCreator.onChange(new SensorDataSet()); } private void mockAltitudeChange(TrackPointCreator trackPointCreator, float altitudeGain) { diff --git a/src/androidTest/java/de/dennisguse/opentracks/services/handlers/GPSHandlerTest.java b/src/androidTest/java/de/dennisguse/opentracks/services/handlers/GPSHandlerTest.java index ecee709d2..baa026196 100644 --- a/src/androidTest/java/de/dennisguse/opentracks/services/handlers/GPSHandlerTest.java +++ b/src/androidTest/java/de/dennisguse/opentracks/services/handlers/GPSHandlerTest.java @@ -22,7 +22,6 @@ import org.mockito.junit.MockitoJUnitRunner; import java.time.Instant; import de.dennisguse.opentracks.data.models.Distance; -import de.dennisguse.opentracks.data.models.TrackPoint; import de.dennisguse.opentracks.settings.PreferencesUtils; @RunWith(MockitoJUnitRunner.class) @@ -62,7 +61,7 @@ public class GPSHandlerTest { locationHandler.onLocationChanged(createLocation(45f, 35f, 3, 5, System.currentTimeMillis())); // then - verify(trackPointCreator, times(1)).onNewTrackPoint(any(TrackPoint.class)); + verify(trackPointCreator, times(1)).onChange(any(Location.class)); } /** @@ -78,7 +77,7 @@ public class GPSHandlerTest { locationHandler.onLocationChanged(createLocation(latitude, 35f, 3, 5, System.currentTimeMillis())); // then - verify(trackPointCreator, times(0)).onNewTrackPoint(any(TrackPoint.class)); + verify(trackPointCreator, times(0)).onChange(any(Location.class)); } /** @@ -94,7 +93,7 @@ public class GPSHandlerTest { // then // no newTrackPoint called - verify(trackPointCreator, times(0)).onNewTrackPoint(any(TrackPoint.class)); + verify(trackPointCreator, times(0)).onChange(any(Location.class)); } @Test @@ -107,7 +106,7 @@ public class GPSHandlerTest { locationHandler.onLocationChanged(createLocation(99.0, 35.0, Long.MAX_VALUE, 15, System.currentTimeMillis())); // then - verify(trackPointCreator, times(1)).onNewTrackPoint(any(TrackPoint.class)); + verify(trackPointCreator, times(1)).onChange(any(Location.class)); } /** diff --git a/src/main/java/de/dennisguse/opentracks/services/handlers/GPSHandler.java b/src/main/java/de/dennisguse/opentracks/services/handlers/GPSHandler.java index 667953959..abf6be108 100644 --- a/src/main/java/de/dennisguse/opentracks/services/handlers/GPSHandler.java +++ b/src/main/java/de/dennisguse/opentracks/services/handlers/GPSHandler.java @@ -12,6 +12,7 @@ import androidx.annotation.NonNull; import androidx.annotation.VisibleForTesting; import java.time.Duration; +import java.time.Instant; import de.dennisguse.opentracks.R; import de.dennisguse.opentracks.data.models.Distance; @@ -109,25 +110,23 @@ public class GPSHandler implements LocationListener, GpsStatus.GpsStatusListener return; } - TrackPoint trackPoint = new TrackPoint(location, trackPointCreator.createNow()); - boolean isAccurate = trackPoint.fulfillsAccuracy(thresholdHorizontalAccuracy); - boolean isValid = LocationUtils.isValidLocation(location); - if (gpsStatus != null) { + // Send each update to the status; please note that this TrackPoint is not stored. + TrackPoint trackPoint = new TrackPoint(location, Instant.ofEpochMilli(location.getTime())); gpsStatus.onLocationChanged(trackPoint); } - if (!isValid) { + if (!LocationUtils.isValidLocation(location)) { Log.w(TAG, "Ignore newTrackPoint. location is invalid."); return; } - if (!isAccurate) { + if (!LocationUtils.fulfillsAccuracy(location, thresholdHorizontalAccuracy)) { Log.d(TAG, "Ignore newTrackPoint. Poor accuracy."); return; } - trackPointCreator.onNewTrackPoint(trackPoint); + trackPointCreator.onChange(location); } @Override diff --git a/src/main/java/de/dennisguse/opentracks/services/handlers/TrackPointCreator.java b/src/main/java/de/dennisguse/opentracks/services/handlers/TrackPointCreator.java index d6dc2da6e..2f583ca2f 100644 --- a/src/main/java/de/dennisguse/opentracks/services/handlers/TrackPointCreator.java +++ b/src/main/java/de/dennisguse/opentracks/services/handlers/TrackPointCreator.java @@ -1,6 +1,7 @@ package de.dennisguse.opentracks.services.handlers; import android.content.Context; +import android.location.Location; import android.util.Log; import android.util.Pair; @@ -47,7 +48,7 @@ public class TrackPointCreator implements BluetoothRemoteSensorManager.SensorDat this.gpsHandler = gpsHandler; } - public void start(@NonNull Context context) { + public synchronized void start(@NonNull Context context) { this.context = context; gpsHandler.onStart(context); @@ -71,7 +72,7 @@ public class TrackPointCreator implements BluetoothRemoteSensorManager.SensorDat gpsHandler.onStop(); } - public void reset() { + public synchronized void reset() { if (remoteSensorManager == null || altitudeSumManager == null) { Log.d(TAG, "No recording running and no reset necessary."); return; @@ -80,7 +81,7 @@ public class TrackPointCreator implements BluetoothRemoteSensorManager.SensorDat altitudeSumManager.reset(); } - private SensorDataSet fill(TrackPoint trackPoint) { + private SensorDataSet addSensorData(TrackPoint trackPoint) { if (!isStarted()) { Log.w(TAG, "Not started, should not be called."); return null; @@ -98,7 +99,7 @@ public class TrackPointCreator implements BluetoothRemoteSensorManager.SensorDat return sensorDataSet; } - public void stop() { + public synchronized void stop() { gpsHandler.onStop(); if (remoteSensorManager != null) { @@ -114,16 +115,21 @@ public class TrackPointCreator implements BluetoothRemoteSensorManager.SensorDat this.context = null; } + public synchronized void onChange(@NonNull Location location) { + onNewTrackPoint(new TrackPoint(location, createNow())); + } + /** * Got a new TrackPoint from Bluetooth only; contains no GPS location. */ @Override - public void onChange(SensorDataSet sensorDataSet) { + public synchronized void onChange(@NonNull SensorDataSet unused) { onNewTrackPoint(new TrackPoint(TrackPoint.Type.SENSORPOINT, createNow())); } - public synchronized void onNewTrackPoint(@NonNull TrackPoint trackPoint) { - fill(trackPoint); + @VisibleForTesting + public void onNewTrackPoint(@NonNull TrackPoint trackPoint) { + addSensorData(trackPoint); boolean stored = service.newTrackPoint(trackPoint, gpsHandler.getThresholdHorizontalAccuracy()); if (stored) { @@ -131,13 +137,13 @@ public class TrackPointCreator implements BluetoothRemoteSensorManager.SensorDat } } - public TrackPoint createSegmentStartManual() { + public synchronized TrackPoint createSegmentStartManual() { return TrackPoint.createSegmentStartManualWithTime(createNow()); } - public TrackPoint createSegmentEnd() { + public synchronized TrackPoint createSegmentEnd() { TrackPoint segmentEnd = TrackPoint.createSegmentEndWithTime(createNow()); - fill(segmentEnd); + addSensorData(segmentEnd); reset(); return segmentEnd; } @@ -158,11 +164,12 @@ public class TrackPointCreator implements BluetoothRemoteSensorManager.SensorDat currentTrackPoint.setLatitude(lastStoredTrackPointWithLocation.getLatitude()); } - SensorDataSet sensorDataSet = fill(currentTrackPoint); + SensorDataSet sensorDataSet = addSensorData(currentTrackPoint); return new Pair<>(currentTrackPoint, sensorDataSet); } + @VisibleForTesting Instant createNow() { return Instant.now(clock); } diff --git a/src/main/java/de/dennisguse/opentracks/util/LocationUtils.java b/src/main/java/de/dennisguse/opentracks/util/LocationUtils.java index 088710b73..d5b28dd31 100644 --- a/src/main/java/de/dennisguse/opentracks/util/LocationUtils.java +++ b/src/main/java/de/dennisguse/opentracks/util/LocationUtils.java @@ -17,6 +17,8 @@ package de.dennisguse.opentracks.util; import android.location.Location; +import de.dennisguse.opentracks.data.models.Distance; + /** * Utility class for decimating tracks at a given level of precision. * @@ -40,4 +42,11 @@ public class LocationUtils { && Math.abs(location.getLatitude()) <= 90 && Math.abs(location.getLongitude()) <= 180; } + + public static boolean fulfillsAccuracy(Location location, Distance thresholdHorizontalAccuracy) { + return location.hasAccuracy() && + Distance.of(location.getAccuracy()) + .lessThan(thresholdHorizontalAccuracy); + + } }