Skip to content

Commit 08b464a

Browse files
triceotriceo
authored
chore: address some Sonar-reported reliability issues (#779)
1 parent 9f46fda commit 08b464a

File tree

12 files changed

+89
-65
lines changed

12 files changed

+89
-65
lines changed

‎core/src/main/java/ai/timefold/solver/core/impl/domain/valuerange/buildin/composite/CompositeCountableValueRange.java‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,14 @@
22

33
import java.util.Iterator;
44
import java.util.List;
5+
import java.util.NoSuchElementException;
56
import java.util.Random;
67
import java.util.Spliterator;
78
import java.util.Spliterators;
89
import java.util.stream.Stream;
910
import java.util.stream.StreamSupport;
1011

1112
import ai.timefold.solver.core.api.domain.valuerange.CountableValueRange;
12-
import ai.timefold.solver.core.api.domain.valuerange.ValueRange;
1313
import ai.timefold.solver.core.impl.domain.valuerange.AbstractCountableValueRange;
1414
import ai.timefold.solver.core.impl.domain.valuerange.util.ValueRangeIterator;
1515
import ai.timefold.solver.core.impl.solver.random.RandomUtils;
@@ -28,10 +28,6 @@ public CompositeCountableValueRange(List<? extends CountableValueRange<T>> child
2828
this.size = size;
2929
}
3030

31-
public List<? extends ValueRange<T>> getChildValueRangeList() {
32-
return childValueRangeList;
33-
}
34-
3531
@Override
3632
public long getSize() {
3733
return size;
@@ -104,8 +100,8 @@ public T next() {
104100
}
105101
remainingIndex -= childSize;
106102
}
107-
throw new IllegalStateException("Impossible state because index (" + index
108-
+ ") is always less than the size (" + size + ").");
103+
throw new NoSuchElementException("Impossible state because index (%d) is always less than the size (%d)."
104+
.formatted(index, size));
109105
}
110106

111107
}

‎core/src/main/java/ai/timefold/solver/core/impl/domain/valuerange/buildin/composite/NullAllowingCountableValueRange.java‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
import java.util.Random;
55

66
import ai.timefold.solver.core.api.domain.valuerange.CountableValueRange;
7-
import ai.timefold.solver.core.api.domain.valuerange.ValueRange;
87
import ai.timefold.solver.core.impl.domain.valuerange.AbstractCountableValueRange;
98
import ai.timefold.solver.core.impl.domain.valuerange.util.ValueRangeIterator;
109
import ai.timefold.solver.core.impl.solver.random.RandomUtils;
@@ -19,10 +18,6 @@ public NullAllowingCountableValueRange(CountableValueRange<T> childValueRange) {
1918
size = childValueRange.getSize() + 1L;
2019
}
2120

22-
public ValueRange<T> getChildValueRange() {
23-
return childValueRange;
24-
}
25-
2621
@Override
2722
public long getSize() {
2823
return size;

‎core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/entity/decorator/ProbabilityEntitySelector.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import java.util.Iterator;
44
import java.util.ListIterator;
5-
import java.util.Map;
65
import java.util.NavigableMap;
76
import java.util.Objects;
87
import java.util.TreeMap;
@@ -106,9 +105,9 @@ public boolean hasNext() {
106105
@Override
107106
public Object next() {
108107
double randomOffset = RandomUtils.nextDouble(workingRandom, probabilityWeightTotal);
109-
Map.Entry<Double, Object> entry = cachedEntityMap.floorEntry(randomOffset);
110108
// entry is never null because randomOffset < probabilityWeightTotal
111-
return entry.getValue();
109+
return cachedEntityMap.floorEntry(randomOffset)
110+
.getValue();
112111
}
113112

114113
@Override

‎core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/list/TriangularNumbers.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,8 @@ public static int nthTriangle(int n) throws ArithmeticException {
2222
}
2323

2424
static double triangularRoot(int x) {
25-
return (Math.sqrt(8L * x + 1) - 1) / 2;
25+
double d = 8L * x + 1;
26+
return (Math.sqrt(d) - 1) / 2;
2627
}
2728

2829
private TriangularNumbers() {

‎core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/move/decorator/ProbabilityMoveSelector.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package ai.timefold.solver.core.impl.heuristic.selector.move.decorator;
22

33
import java.util.Iterator;
4-
import java.util.Map;
54
import java.util.NavigableMap;
65
import java.util.TreeMap;
76

@@ -98,9 +97,9 @@ public boolean hasNext() {
9897
@Override
9998
public Move<Solution_> next() {
10099
double randomOffset = RandomUtils.nextDouble(workingRandom, probabilityWeightTotal);
101-
Map.Entry<Double, Move<Solution_>> entry = cachedMoveMap.floorEntry(randomOffset);
102100
// entry is never null because randomOffset < probabilityWeightTotal
103-
return entry.getValue();
101+
return cachedMoveMap.floorEntry(randomOffset)
102+
.getValue();
104103
}
105104

106105
@Override

‎core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/value/decorator/ProbabilityValueSelector.java‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package ai.timefold.solver.core.impl.heuristic.selector.value.decorator;
22

33
import java.util.Iterator;
4-
import java.util.Map;
54
import java.util.NavigableMap;
65
import java.util.Objects;
76
import java.util.TreeMap;
@@ -25,8 +24,8 @@ public final class ProbabilityValueSelector<Solution_>
2524
private final SelectionCacheType cacheType;
2625
private final SelectionProbabilityWeightFactory<Solution_, Object> probabilityWeightFactory;
2726

28-
protected NavigableMap<Double, Object> cachedEntityMap = null;
29-
protected double probabilityWeightTotal = -1.0;
27+
private NavigableMap<Double, Object> cachedEntityMap = null;
28+
private double probabilityWeightTotal = -1.0;
3029

3130
public ProbabilityValueSelector(EntityIndependentValueSelector<Solution_> childValueSelector,
3231
SelectionCacheType cacheType,
@@ -116,9 +115,9 @@ public boolean hasNext() {
116115
@Override
117116
public Object next() {
118117
double randomOffset = RandomUtils.nextDouble(workingRandom, probabilityWeightTotal);
119-
Map.Entry<Double, Object> entry = cachedEntityMap.floorEntry(randomOffset);
120118
// entry is never null because randomOffset < probabilityWeightTotal
121-
return entry.getValue();
119+
return cachedEntityMap.floorEntry(randomOffset)
120+
.getValue();
122121
}
123122

124123
@Override

‎core/src/main/java/ai/timefold/solver/core/impl/solver/scope/SolverScope.java‎

Lines changed: 50 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import java.util.Set;
88
import java.util.concurrent.ConcurrentHashMap;
99
import java.util.concurrent.Semaphore;
10+
import java.util.concurrent.atomic.AtomicLong;
1011
import java.util.concurrent.atomic.AtomicReference;
1112

1213
import ai.timefold.solver.core.api.domain.solution.PlanningSolution;
@@ -28,31 +29,45 @@
2829
* @param <Solution_> the solution type, the class with the {@link PlanningSolution} annotation
2930
*/
3031
public class SolverScope<Solution_> {
31-
protected Set<SolverMetric> solverMetricSet;
32-
protected Tags monitoringTags;
33-
protected int startingSolverCount;
34-
protected Random workingRandom;
35-
protected InnerScoreDirector<Solution_, ?> scoreDirector;
32+
33+
// Solution-derived fields have the potential for race conditions.
34+
private final AtomicReference<ProblemSizeStatistics> problemSizeStatistics = new AtomicReference<>();
35+
private final AtomicReference<Solution_> bestSolution = new AtomicReference<>();
36+
private final AtomicReference<Score<?>> bestScore = new AtomicReference<>();
37+
private final AtomicLong startingSystemTimeMillis = resetAtomicLongTimeMillis(new AtomicLong());
38+
private final AtomicLong endingSystemTimeMillis = resetAtomicLongTimeMillis(new AtomicLong());
39+
40+
private Set<SolverMetric> solverMetricSet;
41+
private Tags monitoringTags;
42+
private int startingSolverCount;
43+
private Random workingRandom;
44+
private InnerScoreDirector<Solution_, ?> scoreDirector;
3645
private DefaultProblemChangeDirector<Solution_> problemChangeDirector;
3746
/**
3847
* Used for capping CPU power usage in multithreaded scenarios.
3948
*/
40-
protected Semaphore runnableThreadSemaphore = null;
49+
private Semaphore runnableThreadSemaphore = null;
50+
51+
private long childThreadsScoreCalculationCount = 0;
4152

42-
protected volatile Long startingSystemTimeMillis;
43-
protected volatile Long endingSystemTimeMillis;
44-
protected long childThreadsScoreCalculationCount = 0;
53+
private Score<?> startingInitializedScore;
4554

46-
protected Score startingInitializedScore;
55+
private Long bestSolutionTimeMillis;
4756

48-
protected volatile ProblemSizeStatistics problemSizeStatistics;
49-
protected volatile Solution_ bestSolution;
50-
protected volatile Score bestScore;
51-
protected Long bestSolutionTimeMillis;
5257
/**
5358
* Used for tracking step score
5459
*/
55-
protected final Map<Tags, List<AtomicReference<Number>>> stepScoreMap = new ConcurrentHashMap<>();
60+
private final Map<Tags, List<AtomicReference<Number>>> stepScoreMap = new ConcurrentHashMap<>();
61+
62+
private static AtomicLong resetAtomicLongTimeMillis(AtomicLong atomicLong) {
63+
atomicLong.set(-1);
64+
return atomicLong;
65+
}
66+
67+
private static Long readAtomicLongTimeMillis(AtomicLong atomicLong) {
68+
var value = atomicLong.get();
69+
return value == -1 ? null : value;
70+
}
5671

5772
// ************************************************************************
5873
// Constructors and simple getters/setters
@@ -115,11 +130,11 @@ public void setRunnableThreadSemaphore(Semaphore runnableThreadSemaphore) {
115130
}
116131

117132
public Long getStartingSystemTimeMillis() {
118-
return startingSystemTimeMillis;
133+
return readAtomicLongTimeMillis(startingSystemTimeMillis);
119134
}
120135

121136
public Long getEndingSystemTimeMillis() {
122-
return endingSystemTimeMillis;
137+
return readAtomicLongTimeMillis(endingSystemTimeMillis);
123138
}
124139

125140
public SolutionDescriptor<Solution_> getSolutionDescriptor() {
@@ -163,7 +178,7 @@ public long getScoreCalculationCount() {
163178
}
164179

165180
public Solution_ getBestSolution() {
166-
return bestSolution;
181+
return bestSolution.get();
167182
}
168183

169184
/**
@@ -173,15 +188,15 @@ public Solution_ getBestSolution() {
173188
* @param bestSolution never null
174189
*/
175190
public void setBestSolution(Solution_ bestSolution) {
176-
this.bestSolution = bestSolution;
191+
this.bestSolution.set(bestSolution);
177192
}
178193

179194
public Score getBestScore() {
180-
return bestScore;
195+
return bestScore.get();
181196
}
182197

183198
public void setBestScore(Score bestScore) {
184-
this.bestScore = bestScore;
199+
this.bestScore.set(bestScore);
185200
}
186201

187202
public Long getBestSolutionTimeMillis() {
@@ -201,37 +216,37 @@ public boolean isMetricEnabled(SolverMetric solverMetric) {
201216
}
202217

203218
public void startingNow() {
204-
startingSystemTimeMillis = System.currentTimeMillis();
205-
endingSystemTimeMillis = null;
219+
startingSystemTimeMillis.set(System.currentTimeMillis());
220+
resetAtomicLongTimeMillis(endingSystemTimeMillis);
206221
}
207222

208223
public Long getBestSolutionTimeMillisSpent() {
209-
return bestSolutionTimeMillis - startingSystemTimeMillis;
224+
return getBestSolutionTimeMillis() - getStartingSystemTimeMillis();
210225
}
211226

212227
public void endingNow() {
213-
endingSystemTimeMillis = System.currentTimeMillis();
228+
endingSystemTimeMillis.set(System.currentTimeMillis());
214229
}
215230

216231
public boolean isBestSolutionInitialized() {
217-
return bestScore.isSolutionInitialized();
232+
return getBestScore().isSolutionInitialized();
218233
}
219234

220235
public long calculateTimeMillisSpentUpToNow() {
221236
long now = System.currentTimeMillis();
222-
return now - startingSystemTimeMillis;
237+
return now - getStartingSystemTimeMillis();
223238
}
224239

225240
public long getTimeMillisSpent() {
226-
return endingSystemTimeMillis - startingSystemTimeMillis;
241+
return getEndingSystemTimeMillis() - getStartingSystemTimeMillis();
227242
}
228243

229244
public ProblemSizeStatistics getProblemSizeStatistics() {
230-
return problemSizeStatistics;
245+
return problemSizeStatistics.get();
231246
}
232247

233248
public void setProblemSizeStatistics(ProblemSizeStatistics problemSizeStatistics) {
234-
this.problemSizeStatistics = problemSizeStatistics;
249+
this.problemSizeStatistics.set(problemSizeStatistics);
235250
}
236251

237252
/**
@@ -249,23 +264,23 @@ public static long getScoreCalculationSpeed(long scoreCalculationCount, long tim
249264

250265
public void setWorkingSolutionFromBestSolution() {
251266
// The workingSolution must never be the same instance as the bestSolution.
252-
scoreDirector.setWorkingSolution(scoreDirector.cloneSolution(bestSolution));
267+
scoreDirector.setWorkingSolution(scoreDirector.cloneSolution(getBestSolution()));
253268
}
254269

255270
public SolverScope<Solution_> createChildThreadSolverScope(ChildThreadType childThreadType) {
256271
SolverScope<Solution_> childThreadSolverScope = new SolverScope<>();
272+
childThreadSolverScope.bestSolution.set(null);
273+
childThreadSolverScope.bestScore.set(null);
257274
childThreadSolverScope.monitoringTags = monitoringTags;
258275
childThreadSolverScope.solverMetricSet = solverMetricSet;
259276
childThreadSolverScope.startingSolverCount = startingSolverCount;
260277
// TODO FIXME use RandomFactory
261278
// Experiments show that this trick to attain reproducibility doesn't break uniform distribution
262279
childThreadSolverScope.workingRandom = new Random(workingRandom.nextLong());
263280
childThreadSolverScope.scoreDirector = scoreDirector.createChildThreadScoreDirector(childThreadType);
264-
childThreadSolverScope.startingSystemTimeMillis = startingSystemTimeMillis;
265-
childThreadSolverScope.endingSystemTimeMillis = null;
281+
childThreadSolverScope.startingSystemTimeMillis.set(startingSystemTimeMillis.get());
282+
resetAtomicLongTimeMillis(childThreadSolverScope.endingSystemTimeMillis);
266283
childThreadSolverScope.startingInitializedScore = null;
267-
childThreadSolverScope.bestSolution = null;
268-
childThreadSolverScope.bestScore = null;
269284
childThreadSolverScope.bestSolutionTimeMillis = null;
270285
return childThreadSolverScope;
271286
}

‎quarkus-integration/quarkus-benchmark/integration-test/src/main/java/ai/timefold/solver/quarkus/benchmark/it/TimefoldBenchmarkTestResource.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,12 @@
1616
@Path("/timefold/test")
1717
public class TimefoldBenchmarkTestResource {
1818

19+
private final PlannerBenchmarkFactory benchmarkFactory;
20+
1921
@Inject
20-
PlannerBenchmarkFactory benchmarkFactory;
22+
public TimefoldBenchmarkTestResource(PlannerBenchmarkFactory benchmarkFactory) {
23+
this.benchmarkFactory = benchmarkFactory;
24+
}
2125

2226
@POST
2327
@Path("/benchmark")

‎quarkus-integration/quarkus-jackson/integration-test/src/main/java/ai/timefold/solver/quarkus/jackson/it/TimefoldTestResource.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,12 @@
1313
@Path("/timefold/test")
1414
public class TimefoldTestResource {
1515

16+
private final SolverManager<ITestdataPlanningSolution, Long> solverManager;
17+
1618
@Inject
17-
SolverManager<ITestdataPlanningSolution, Long> solverManager;
19+
public TimefoldTestResource(SolverManager<ITestdataPlanningSolution, Long> solverManager) {
20+
this.solverManager = solverManager;
21+
}
1822

1923
@POST
2024
@Path("/solver-factory")

‎quarkus-integration/quarkus-jsonb/integration-test/src/main/java/ai/timefold/solver/quarkus/jsonb/it/TimefoldTestResource.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,12 @@
1313
@Path("/timefold/test")
1414
public class TimefoldTestResource {
1515

16+
private final SolverManager<ITestdataPlanningSolution, Long> solverManager;
17+
1618
@Inject
17-
SolverManager<ITestdataPlanningSolution, Long> solverManager;
19+
public TimefoldTestResource(SolverManager<ITestdataPlanningSolution, Long> solverManager) {
20+
this.solverManager = solverManager;
21+
}
1822

1923
@POST
2024
@Path("/solver-factory")

0 commit comments

Comments
 (0)