Hi Orekit team,
first of all thanks for your amazing framework.
I think we found small bug in version 12.0.1.
The scenario
- We have long living services (a component that is serving e.g. a REST API)
- We have a cronjob that refreshes the orekit context every now and then.
- We ensure everywhere that once you start a calculation the latest version of the context is used and that exactly that instance of the context is used during the whole instance of that calculation. This makes it possible to replace the kontext without side effects.
- Our services are responding to a lot of requests, so it is very busy / always calculating.
We also tested this extensively. E.g. a test that does a lot of calculations and then replaces the context often, kind of a small stress test. Here is the source code of the test:
@Test
@SneakyThrows
void testThatDataContextRefreshWorksDuringCalculations() {
val futures =
new CompletableFuture<?>[] {
supplyAsync(this::refresh),
supplyAsync(this::executeManyConversionWithOrekit),
supplyAsync(this::executeManyConversionWithOrekit),
supplyAsync(this::executeManyConversionWithOrekit),
supplyAsync(this::executeManyConversionWithOrekit),
supplyAsync(this::executeManyConversionWithOrekit),
};
CompletableFuture.allOf(futures).get();
}
private boolean executeManyConversionWithOrekit() {
val rand = new Random();
for (int i = 0; i < 100000; i++) {
val epoch = new Epoch(rand.nextInt(500000));
// Some conversion from a proprietary time implementation making use of time scales.
val offsetDateTime = epochToOffsetDateTime(DataContext.getDefault(), epoch);
assertNotNull(offsetDateTime);
}
return true;
}
@SneakyThrows
private boolean refresh() {
for (int i = 0; i < 20; i++) {
replaceOrekitDataProvider("./src/test/resources/orekit-data");
Thread.sleep(100);
}
return true;
}
public static void replaceOrekitDataProvider(String orekitDataFolder) {
log.info("Replacing orekit data path with {}", orekitDataFolder);
val lazyLoadedDataContext = new LazyLoadedDataContext();
lazyLoadedDataContext
.getDataProvidersManager()
.addProvider(new DirectoryCrawler(new File(orekitDataFolder)));
DataContext.setDefault(lazyLoadedDataContext);
log.info("Replaced orekit data provider.");
}
The problem:
We are now upgrading to from 11.3 to version 12.0.1 and since then this test does not work anymore. This test now fails with the following error:
Caused by: java.util.ConcurrentModificationException
at java.base/java.util.ArrayList$Itr.checkForComodification(ArrayList.java:1013)
at java.base/java.util.ArrayList$Itr.next(ArrayList.java:967)
at org.orekit.time.LazyLoadedTimeScales.getUTC(LazyLoadedTimeScales.java:184)
// ... some not relevant stacks that are super private
at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1768)
at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.exec(CompletableFuture.java:1760)
at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:373)
at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1182)
at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1655)
at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1622)
at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:165)
I would say that the problem is that you changed the synchronization behavior in this commit: https://gitlab.orekit.org/orekit/orekit/-/commit/a1c70a192097e6c1b8945fb9994182e12a75ede7
It looks like the iteration over the loaders are not synchronized. This is an issue for us, because one thread will replace the context and many other threads shortly spawned after that concurrently will all try to initialize the loaders.
This is probably the case for all LazyLoaded... classes.
Ideas for possible solutions
- Synchronize reading access to
loadersas it was before. - For our exact scenario we could think of something like an
ImmutableContext, that is - not lazy loaded and also the loaders are not changeable. It needs to be initialized and loaded and only then made available. Such a context wouldn’t have to be synchronized at all interactions are read only, hence improving also performance. - We have a wrapper around your
DataContextin order to have convenience methods etc., so we could synchronize access there, but of course we would like to try to avoid that.
Looking forward to your feedback.
Thanks and regards,
Guillermo