From ee79e935d4edc9c2f4df0e9be5652c353a71e26a Mon Sep 17 00:00:00 2001 From: fred Date: Wed, 7 May 2025 11:52:40 -0300 Subject: [PATCH 1/5] fix: clone the solution before sending it to the consumer --- .../core/impl/solver/DefaultSolver.java | 2 +- .../solver/recaller/BestSolutionRecaller.java | 16 +++++++--- .../core/impl/solver/scope/SolverScope.java | 6 +++- .../impl/solver/BestSolutionHolderTest.java | 31 +++++++++++++++++++ 4 files changed, 48 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/ai/timefold/solver/core/impl/solver/DefaultSolver.java b/core/src/main/java/ai/timefold/solver/core/impl/solver/DefaultSolver.java index 87d917905d3..6feb998fec9 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/solver/DefaultSolver.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/solver/DefaultSolver.java @@ -204,7 +204,7 @@ public void setMonitorTagMap(Map monitorTagMap) { restartSolver = checkProblemFactChanges(); } outerSolvingEnded(solverScope); - return solverScope.getBestSolution(); + return solverScope.cloneBestSolution(); } public void outerSolvingStarted(SolverScope solverScope) { diff --git a/core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecaller.java b/core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecaller.java index da841efcfe6..6fba0f1d1c0 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecaller.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecaller.java @@ -118,19 +118,26 @@ public > void processWorkingSolutionDuringMove(Inne public void updateBestSolutionAndFire(SolverScope solverScope) { updateBestSolutionWithoutFiring(solverScope); - solverEventSupport.fireBestSolutionChanged(solverScope, solverScope.getBestSolution()); + // We need to clone the solution again, or we may use the same instance employed by the solver scope, + // which is restored between solver phases. + // The operation won't block the solving process. + solverEventSupport.fireBestSolutionChanged(solverScope, solverScope.cloneBestSolution()); } public void updateBestSolutionAndFireIfInitialized(SolverScope solverScope) { updateBestSolutionWithoutFiring(solverScope); if (solverScope.isBestSolutionInitialized()) { - solverEventSupport.fireBestSolutionChanged(solverScope, solverScope.getBestSolution()); + // We need to clone the solution again, or we may use the same instance employed by the solver scope, + // which is restored between solver phases. + // The operation won't block the solving process. + solverEventSupport.fireBestSolutionChanged(solverScope, solverScope.cloneBestSolution()); } } private void updateBestSolutionAndFire(SolverScope solverScope, InnerScore bestScore, Solution_ bestSolution) { updateBestSolutionWithoutFiring(solverScope, bestScore, bestSolution); + // There is no need to clone the solution because the caller has already done that solverEventSupport.fireBestSolutionChanged(solverScope, solverScope.getBestSolution()); } @@ -144,11 +151,10 @@ private void updateBestSolutionWithoutFiring(SolverScope solverScope) private void updateBestSolutionWithoutFiring(SolverScope solverScope, InnerScore bestScore, Solution_ bestSolution) { - if (bestScore.fullyAssigned()) { - if (!solverScope.isBestSolutionInitialized()) { + if (bestScore.fullyAssigned() && !solverScope.isBestSolutionInitialized()) { solverScope.setStartingInitializedScore(bestScore.raw()); } - } + solverScope.setBestSolution(bestSolution); solverScope.setBestScore(bestScore); solverScope.setBestSolutionTimeMillis(solverScope.getClock().millis()); diff --git a/core/src/main/java/ai/timefold/solver/core/impl/solver/scope/SolverScope.java b/core/src/main/java/ai/timefold/solver/core/impl/solver/scope/SolverScope.java index 8305c56dfcc..20f9f83cf7b 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/solver/scope/SolverScope.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/solver/scope/SolverScope.java @@ -224,6 +224,10 @@ public Solution_ getBestSolution() { return bestSolution.get(); } + public Solution_ cloneBestSolution() { + return scoreDirector.cloneSolution(bestSolution.get()); + } + /** * The {@link PlanningSolution best solution} must never be the same instance * as the {@link PlanningSolution working solution}, it should be a (un)changed clone. @@ -332,7 +336,7 @@ public long getMoveEvaluationSpeed() { public void setWorkingSolutionFromBestSolution() { // The workingSolution must never be the same instance as the bestSolution. - scoreDirector.setWorkingSolution(scoreDirector.cloneSolution(getBestSolution())); + scoreDirector.setWorkingSolution(cloneBestSolution()); } public SolverScope createChildThreadSolverScope(ChildThreadType childThreadType) { diff --git a/core/src/test/java/ai/timefold/solver/core/impl/solver/BestSolutionHolderTest.java b/core/src/test/java/ai/timefold/solver/core/impl/solver/BestSolutionHolderTest.java index 7c18f79254c..03fa176464d 100644 --- a/core/src/test/java/ai/timefold/solver/core/impl/solver/BestSolutionHolderTest.java +++ b/core/src/test/java/ai/timefold/solver/core/impl/solver/BestSolutionHolderTest.java @@ -13,14 +13,17 @@ import java.util.UUID; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutionException; import java.util.concurrent.Executors; import ai.timefold.solver.core.api.solver.Solver; +import ai.timefold.solver.core.api.solver.SolverConfigOverride; import ai.timefold.solver.core.api.solver.SolverManager; import ai.timefold.solver.core.api.solver.change.ProblemChange; import ai.timefold.solver.core.api.solver.change.ProblemChangeDirector; import ai.timefold.solver.core.config.solver.SolverConfig; import ai.timefold.solver.core.config.solver.SolverManagerConfig; +import ai.timefold.solver.core.config.solver.termination.TerminationConfig; import ai.timefold.solver.core.testdomain.TestdataEasyScoreCalculator; import ai.timefold.solver.core.testdomain.TestdataEntity; import ai.timefold.solver.core.testdomain.TestdataSolution; @@ -186,6 +189,34 @@ void problemChangeBarrageIntermediateBestSolutionConsumer() throws InterruptedEx } + @Test + void testCloningBetweenPhases() throws ExecutionException, InterruptedException { + var solverConfig = new SolverConfig() + .withSolutionClass(TestdataSolution.class) + .withEntityClasses(TestdataEntity.class) + .withEasyScoreCalculatorClass(TestdataEasyScoreCalculator.class); + + try (var solverManager = SolverManager. create(solverConfig, new SolverManagerConfig())) { + var problem = TestdataSolution.generateSolution(2, 2); + problem.getEntityList().forEach(entity -> entity.setValue(null)); + var countDownLatch = new CountDownLatch(1); + var solution = solverManager.solveBuilder() + .withProblemId(UUID.randomUUID()) + .withProblem(problem) + .withBestSolutionConsumer( + intermediate -> { + intermediate.getEntityList().add(new TestdataEntity("Bad Entity 3")); + countDownLatch.countDown(); + }) + .withConfigOverride(new SolverConfigOverride() + .withTerminationConfig(new TerminationConfig().withMoveCountLimit(100L))) + .run() + .getFinalBestSolution(); + countDownLatch.await(); + assertThat(solution.getEntityList()).hasSize(2); + } + } + private record RecordedFuture(int id, CompletableFuture future) { boolean isDone() { From 14d06b877643318ca14f7a6560c1fc9dbf10b569 Mon Sep 17 00:00:00 2001 From: fred Date: Wed, 7 May 2025 12:27:24 -0300 Subject: [PATCH 2/5] fix: formatting --- .../core/impl/solver/recaller/BestSolutionRecaller.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecaller.java b/core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecaller.java index 6fba0f1d1c0..58e964627a4 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecaller.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecaller.java @@ -152,8 +152,8 @@ private void updateBestSolutionWithoutFiring(SolverScope solverScope) private void updateBestSolutionWithoutFiring(SolverScope solverScope, InnerScore bestScore, Solution_ bestSolution) { if (bestScore.fullyAssigned() && !solverScope.isBestSolutionInitialized()) { - solverScope.setStartingInitializedScore(bestScore.raw()); - } + solverScope.setStartingInitializedScore(bestScore.raw()); + } solverScope.setBestSolution(bestSolution); solverScope.setBestScore(bestScore); From f565b02b15b19e427db1f7cb9fdbb1c47861f6e1 Mon Sep 17 00:00:00 2001 From: fred Date: Wed, 7 May 2025 13:26:23 -0300 Subject: [PATCH 3/5] fix: test --- .../DefaultConstructionHeuristicPhaseTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/src/test/java/ai/timefold/solver/core/impl/constructionheuristic/DefaultConstructionHeuristicPhaseTest.java b/core/src/test/java/ai/timefold/solver/core/impl/constructionheuristic/DefaultConstructionHeuristicPhaseTest.java index 058625f2dd2..e86fadf5dd0 100644 --- a/core/src/test/java/ai/timefold/solver/core/impl/constructionheuristic/DefaultConstructionHeuristicPhaseTest.java +++ b/core/src/test/java/ai/timefold/solver/core/impl/constructionheuristic/DefaultConstructionHeuristicPhaseTest.java @@ -91,7 +91,8 @@ void solveWithInitializedSolution() { new TestdataEntity("e3", v3))); var solution = PlannerTestUtils.solve(solverConfig, inputProblem, false); - assertThat(inputProblem).isSameAs(solution); + assertThat(inputProblem.getEntityList().stream().map(TestdataEntity::getValue).toList()) + .isEqualTo(solution.getEntityList().stream().map(TestdataEntity::getValue).toList()); } @Test From 5aa4fa645b362bdacf4ae54a544721f81ac7fcec Mon Sep 17 00:00:00 2001 From: fred Date: Wed, 7 May 2025 13:33:53 -0300 Subject: [PATCH 4/5] fix: clone the solution before sending it to the consumer --- .../core/impl/solver/recaller/BestSolutionRecaller.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecaller.java b/core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecaller.java index 58e964627a4..7816faa0a36 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecaller.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecaller.java @@ -137,8 +137,10 @@ public void updateBestSolutionAndFireIfInitialized(SolverScope solver private void updateBestSolutionAndFire(SolverScope solverScope, InnerScore bestScore, Solution_ bestSolution) { updateBestSolutionWithoutFiring(solverScope, bestScore, bestSolution); - // There is no need to clone the solution because the caller has already done that - solverEventSupport.fireBestSolutionChanged(solverScope, solverScope.getBestSolution()); + // We need to clone the solution again, or we may use the same instance employed by the solver scope, + // which is restored between solver phases. + // The operation won't block the solving process. + solverEventSupport.fireBestSolutionChanged(solverScope, solverScope.cloneBestSolution()); } @SuppressWarnings({ "unchecked", "rawtypes" }) From 670bda7d53d81f702b8f5f899dd827bdc683dcc0 Mon Sep 17 00:00:00 2001 From: fred Date: Mon, 19 May 2025 10:31:42 -0300 Subject: [PATCH 5/5] chore: address comments --- .../impl/solver/event/SolverEventSupport.java | 5 ++++- .../solver/recaller/BestSolutionRecaller.java | 16 ++++------------ .../core/impl/solver/BestSolutionHolderTest.java | 2 ++ 3 files changed, 10 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/ai/timefold/solver/core/impl/solver/event/SolverEventSupport.java b/core/src/main/java/ai/timefold/solver/core/impl/solver/event/SolverEventSupport.java index 8a91bf010cc..a1357b1504b 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/solver/event/SolverEventSupport.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/solver/event/SolverEventSupport.java @@ -24,7 +24,10 @@ public void fireBestSolutionChanged(SolverScope solverScope, Solution var timeMillisSpent = solverScope.getBestSolutionTimeMillisSpent(); var bestScore = solverScope.getBestScore(); if (it.hasNext()) { - var event = new BestSolutionChangedEvent<>(solver, timeMillisSpent, newBestSolution, bestScore.raw(), + // We need to clone the new best solution, or we may share the same instance with user consumers. + // Reusing the instance can lead to inconsistent states if intermediary consumers change the solution. + var newBestSolutionCloned = solverScope.getScoreDirector().cloneSolution(newBestSolution); + var event = new BestSolutionChangedEvent<>(solver, timeMillisSpent, newBestSolutionCloned, bestScore.raw(), bestScore.fullyAssigned()); do { it.next().bestSolutionChanged(event); diff --git a/core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecaller.java b/core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecaller.java index 7816faa0a36..ac7fe8b5808 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecaller.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecaller.java @@ -118,33 +118,25 @@ public > void processWorkingSolutionDuringMove(Inne public void updateBestSolutionAndFire(SolverScope solverScope) { updateBestSolutionWithoutFiring(solverScope); - // We need to clone the solution again, or we may use the same instance employed by the solver scope, - // which is restored between solver phases. - // The operation won't block the solving process. - solverEventSupport.fireBestSolutionChanged(solverScope, solverScope.cloneBestSolution()); + solverEventSupport.fireBestSolutionChanged(solverScope, solverScope.getBestSolution()); } public void updateBestSolutionAndFireIfInitialized(SolverScope solverScope) { updateBestSolutionWithoutFiring(solverScope); if (solverScope.isBestSolutionInitialized()) { - // We need to clone the solution again, or we may use the same instance employed by the solver scope, - // which is restored between solver phases. - // The operation won't block the solving process. - solverEventSupport.fireBestSolutionChanged(solverScope, solverScope.cloneBestSolution()); + solverEventSupport.fireBestSolutionChanged(solverScope, solverScope.getBestSolution()); } } private void updateBestSolutionAndFire(SolverScope solverScope, InnerScore bestScore, Solution_ bestSolution) { updateBestSolutionWithoutFiring(solverScope, bestScore, bestSolution); - // We need to clone the solution again, or we may use the same instance employed by the solver scope, - // which is restored between solver phases. - // The operation won't block the solving process. - solverEventSupport.fireBestSolutionChanged(solverScope, solverScope.cloneBestSolution()); + solverEventSupport.fireBestSolutionChanged(solverScope, bestSolution); } @SuppressWarnings({ "unchecked", "rawtypes" }) private void updateBestSolutionWithoutFiring(SolverScope solverScope) { + // We clone the existing working solution to set it as the best current solution var newBestSolution = solverScope.getScoreDirector().cloneWorkingSolution(); var newBestScore = solverScope.getSolutionDescriptor(). getScore(newBestSolution); var innerScore = InnerScore.withUnassignedCount(newBestScore, -solverScope.getScoreDirector().getWorkingInitScore()); diff --git a/core/src/test/java/ai/timefold/solver/core/impl/solver/BestSolutionHolderTest.java b/core/src/test/java/ai/timefold/solver/core/impl/solver/BestSolutionHolderTest.java index 03fa176464d..f11baee1ddd 100644 --- a/core/src/test/java/ai/timefold/solver/core/impl/solver/BestSolutionHolderTest.java +++ b/core/src/test/java/ai/timefold/solver/core/impl/solver/BestSolutionHolderTest.java @@ -205,6 +205,7 @@ void testCloningBetweenPhases() throws ExecutionException, InterruptedException .withProblem(problem) .withBestSolutionConsumer( intermediate -> { + // Add a new entity to the cloned solution intermediate.getEntityList().add(new TestdataEntity("Bad Entity 3")); countDownLatch.countDown(); }) @@ -213,6 +214,7 @@ void testCloningBetweenPhases() throws ExecutionException, InterruptedException .run() .getFinalBestSolution(); countDownLatch.await(); + // At this point, the events generated by the best solution consumer should not add any new entities assertThat(solution.getEntityList()).hasSize(2); } }