Bug 1118784

Summary:	NPE when getBestScoreImproved method is called from AbstractStepScope during ConstructionHeuristic
Product:	[Retired] JBoss BRMS Platform 6	Reporter:	jvahala
Component:	OptaPlanner	Assignee:	Geoffrey De Smet <gdesmet>
Status:	CLOSED WONTFIX	QA Contact:	jvahala
Severity:	low	Docs Contact:
Priority:	medium
Version:	6.0.2	CC:	kverlaen, lpetrovi
Target Milestone:	---
Target Release:	---
Hardware:	Unspecified
OS:	Unspecified
Whiteboard:
Fixed In Version:		Doc Type:	Bug Fix
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2014-07-14 12:27:44 UTC	Type:	Bug
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:

Description jvahala 2014-07-11 14:10:29 UTC

Description of problem:
When I register my own step listener and I ask AbstractStepScope if score was improved NPE is thrown during Construction Heuristic phase. The problem is that  bestScoreImproved attribute is initialized after first LocalSearch step. But technically it should be false from the beginning because ConstructionHeuristic always lowers score. 

How reproducible:
Extends SolverPhaseLifecycleListener and in stepEnded method use for example really simple IF:  if(stepScope.getBestScoreImproved().booleanValue())

Actual results:
NPE is thrown

Expected results:
result of calling .getBestScoreImproved().booleanValue() should be false

Comment 2 Geoffrey De Smet 2014-07-11 14:23:23 UTC

This is a known issue (which might not be solvable even). Note that LocalSearchPhaseLifecycleListener (unlike SolverEventListener) isn't part of the public API because they are deep inside the optaplanner-core. Despite that, the issue is valid of course.

Cause:
In every step, the Construction Heuristics initialize 1 entity. Because this causes the uninitializedEntityCount to decrease, this actually means that the best score improved(!). Think about it, a solution with score -7 and 10 unassigned nurses is better than a solution with score 0 and 20 unassigned nurses.
However, this would imply that best solution changed, so the working solution needs to be cloned and an best solution event needs to be fired. That cloning is a huge performance killer. So CH's don't do that, they delay that until the last step...
Meanwhile, the bestSolutionImproved and other related field stay null to fail fast if any code is using them (which is better than holding the wrong non-null value...)

Comment 3 Geoffrey De Smet 2014-07-14 12:19:49 UTC

Going to reject this issue because reasons stated above and more importantly because the code does:
  ((DefaultSolver) solver).addSolverPhaseLifecycleListener(listener);

Downcasting to DefaultSolver, to be able to mess with the internal classes is not supported.

Meanwhile, Jiri's PR will improve the javadoc for those that do mess with the internal classes. Also, we 're investigating why there was the need to mess with the internal classes in the first place, and if we remove that need by supporting a new requirement on the public api.