Skip to content

Commit aed53f4

Browse files
committed
Attempt to fix JOSM Jenkins test failures
The problem has been irreproducible outside of Jenkins, so this is a "best guess" fix. What is likely happening is a downloader has not finished downloading before the test completes, but does finish after the layer cleanup occurs. Signed-off-by: Taylor Smock <tsmock@meta.com>
1 parent eaa3a2f commit aed53f4

File tree

4 files changed

+46
-1
lines changed

4 files changed

+46
-1
lines changed

src/main/java/org/openstreetmap/josm/plugins/mapillary/gui/workers/MapillaryNodeDownloader.java

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
// License: GPL. For details, see LICENSE file.
22
package org.openstreetmap.josm.plugins.mapillary.gui.workers;
33

4+
import java.util.ArrayList;
45
import java.util.Collection;
6+
import java.util.List;
57
import java.util.Objects;
68
import java.util.concurrent.ExecutionException;
79
import java.util.function.Consumer;
@@ -16,6 +18,7 @@
1618
* Download a singular node. This is faster than downloading large sequences.
1719
*/
1820
public class MapillaryNodeDownloader extends MapillaryUIDownloader<MapillaryNode, Void> {
21+
private static final List<MapillaryNodeDownloader> downloadList = new ArrayList<>();
1922
private final long node;
2023
private final Consumer<MapillaryNode> onFinish;
2124

@@ -37,6 +40,7 @@ public MapillaryNodeDownloader(INode node, Consumer<MapillaryNode> onFinish) {
3740
*/
3841
public MapillaryNodeDownloader(long id, Consumer<MapillaryNode> onFinish) {
3942
Objects.requireNonNull(onFinish);
43+
downloadList.add(this);
4044
this.node = id;
4145
this.onFinish = onFinish;
4246
}
@@ -59,4 +63,13 @@ protected void done() {
5963
throw new JosmRuntimeException(e);
6064
}
6165
}
66+
67+
/**
68+
* Kill all download threads
69+
*/
70+
static void killAll() {
71+
for (MapillaryNodeDownloader downloader : downloadList) {
72+
downloader.cancel(true);
73+
}
74+
}
6275
}

src/main/java/org/openstreetmap/josm/plugins/mapillary/gui/workers/MapillaryNodesDownloader.java

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// License: GPL. For details, see LICENSE file.
22
package org.openstreetmap.josm.plugins.mapillary.gui.workers;
33

4+
import java.util.ArrayList;
45
import java.util.List;
56
import java.util.Objects;
67
import java.util.concurrent.ExecutionException;
@@ -15,12 +16,14 @@
1516
* Download nodes asynchronously
1617
*/
1718
public class MapillaryNodesDownloader extends MapillaryUIDownloader<List<MapillaryNode>, Void> {
19+
private static final List<MapillaryNodesDownloader> downloadList = new ArrayList<>();
1820
private final Consumer<List<MapillaryNode>> onFinish;
1921
private final long[] images;
2022

2123
public MapillaryNodesDownloader(Consumer<List<MapillaryNode>> onFinish, long... images) {
2224
Objects.requireNonNull(onFinish);
2325
Objects.requireNonNull(images);
26+
downloadList.add(this);
2427
this.onFinish = onFinish;
2528
this.images = images.clone();
2629
}
@@ -33,14 +36,29 @@ protected List<MapillaryNode> doInBackground() {
3336

3437
@Override
3538
protected void done() {
36-
super.done();
3739
try {
40+
super.done();
3841
this.onFinish.accept(this.get());
3942
} catch (InterruptedException e) {
4043
Thread.currentThread().interrupt();
4144
throw new JosmRuntimeException(e);
4245
} catch (ExecutionException e) {
4346
throw new JosmRuntimeException(e);
47+
} finally {
48+
downloadList.remove(this);
49+
}
50+
}
51+
52+
/**
53+
* Kill all download threads
54+
*/
55+
public static void killAll() {
56+
// Kill the sequence downloader first -- it may cause nodes to be downloaded
57+
MapillarySequenceDownloader.killAll();
58+
// Kill individual node downloads
59+
MapillaryNodeDownloader.killAll();
60+
for (MapillaryNodesDownloader downloader : downloadList) {
61+
downloader.cancel(true);
4462
}
4563
}
4664
}

src/main/java/org/openstreetmap/josm/plugins/mapillary/gui/workers/MapillarySequenceDownloader.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,16 @@ protected static synchronized void updateCurrentSequenceDownload(
5050
currentSequence = mapillarySequenceDownloader;
5151
}
5252

53+
/**
54+
* Kill all sequence downloads
55+
*/
56+
static synchronized void killAll() {
57+
if (currentSequence != null) {
58+
currentSequence.cancel(true);
59+
currentSequence = null;
60+
}
61+
}
62+
5363
/**
5464
* Create a new sequence downloader
5565
*

test/unit/org/openstreetmap/josm/plugins/mapillary/io/remotecontrol/MapillaryRemoteControlTest.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import org.openstreetmap.josm.io.remotecontrol.RemoteControl;
2525
import org.openstreetmap.josm.io.remotecontrol.handler.RequestHandler;
2626
import org.openstreetmap.josm.plugins.mapillary.gui.layer.MapillaryLayer;
27+
import org.openstreetmap.josm.plugins.mapillary.gui.workers.MapillaryNodesDownloader;
2728
import org.openstreetmap.josm.plugins.mapillary.testutils.annotations.AwaitThreadFinish;
2829
import org.openstreetmap.josm.plugins.mapillary.testutils.annotations.MapillaryCaches;
2930
import org.openstreetmap.josm.plugins.mapillary.testutils.annotations.MapillaryLayerAnnotation;
@@ -119,6 +120,8 @@ void testHandleRequest(final String request, final boolean createLayerFirst)
119120
mapillaryRemoteControl.validateRequest();
120121
mapillaryRemoteControl.handleRequest();
121122

123+
Awaitility.await().atMost(Durations.TEN_SECONDS)
124+
.until(() -> !MainApplication.getLayerManager().getLayersOfType(MapillaryLayer.class).isEmpty());
122125
Awaitility.await().atMost(Durations.TEN_SECONDS).until(() -> MapillaryLayer.getInstance().getImage() != null);
123126

124127
final String id = request.replaceAll(".*=Mapillary/", "");
@@ -128,6 +131,7 @@ void testHandleRequest(final String request, final boolean createLayerFirst)
128131
assertEquals(id, MapillaryImageUtils.getSequenceKey(MapillaryLayer.getInstance().getImage()));
129132
}
130133
assertEquals(1, MainApplication.getLayerManager().getLayersOfType(MapillaryLayer.class).size());
134+
MapillaryNodesDownloader.killAll();
131135
}
132136

133137
@Test

0 commit comments

Comments
 (0)