Closed uli-f closed 1 year ago
@uli-f , I would like to point out, that here again the code design is take directly from cdk-inchi. By adding inner static class instead of singleton implementation, it will be a small difference in the design, but I suppose it will be OK for the CDK people?
@uli-f , I would like to point out, that here again the code design is take directly from cdk-inchi. By adding inner static class instead of singleton implementation, it will be a small difference in the design, but I suppose it will be OK for the CDK people?
Thank you for being so vigilant 👀
The singleton pattern that was in place is not entirely thread safe, so I - supposedly - improved it.
I will take responsibility for negotiating these implementation decisions with John and Egon 😃
@uli-f , after applying the first commit 67e51de456f4f572560fc3c0430b11124032d9fa, a problem appears in class RInChIGeneratorFactoryTest in the following test method:
public void testGetInstance_threadSafety() throws InterruptedException, CDKException {
RInChIGeneratorFactory singletonInstance = RInChIGeneratorFactory.getInstance();
int numberOfMethodCalls = 10000;
ConcurrentLinkedQueue<RInChIGeneratorFactory> factoryInstancesQueue = new ConcurrentLinkedQueue<>();
ExecutorService executorService = Executors.newFixedThreadPool(numberOfMethodCalls / 10);
for (int i = 0; i < numberOfMethodCalls; i++) {
executorService.execute(() -> {
try {
RInChIGeneratorFactory factory = RInChIGeneratorFactory.getInstance();
factoryInstancesQueue.add(factory);
} catch (CDKException exception) {
fail("Exception when instantiating RInChIGeneratorFactory: " + exception.getClass() + ", " + exception.getMessage());
}
});
}
executorService.shutdown();
assertTrue(executorService.awaitTermination(5, TimeUnit.SECONDS));
Assert.assertEquals(numberOfMethodCalls, factoryInstancesQueue.size());
for (RInChIGeneratorFactory factory: factoryInstancesQueue) {
assertSame(singletonInstance, factory);
}
}
The error is: Unreachable catch block for CDKException. This exception is never thrown from the try statement body The problem could be solved by removing the try - catch. Should I proceed with your commit and additionally remove the try-catch e.g. update the code to be:
for (int i = 0; i < numberOfMethodCalls; i++) {
executorService.execute(() -> {
RInChIGeneratorFactory factory = RInChIGeneratorFactory.getInstance();
factoryInstancesQueue.add(factory);
});
}
Yes, please proceed and remove the try-catch.
Sorry about that and thanks for sorting this out!
Merged only commit 67e51de456f4f572560fc3c0430b11124032d9fa as commit dd5055c789d490d5f6bf8c57f3ec6fc326f52123. Pushed additional commit, ac5ceaed52348a8e34b959bcba3a2e5e654d8e50, to resolve the try-catch problem. Indentation changes will be done later.
I made it two commits again: one for the functional changes and one for the indentation.