diff --git a/gui/admin-gui/src/main/java/com/evolveum/midpoint/web/page/admin/roles/AbstractRoleMemberPanel.java b/gui/admin-gui/src/main/java/com/evolveum/midpoint/web/page/admin/roles/AbstractRoleMemberPanel.java index ca2d270f618..656ef496a95 100644 --- a/gui/admin-gui/src/main/java/com/evolveum/midpoint/web/page/admin/roles/AbstractRoleMemberPanel.java +++ b/gui/admin-gui/src/main/java/com/evolveum/midpoint/web/page/admin/roles/AbstractRoleMemberPanel.java @@ -69,7 +69,6 @@ import com.evolveum.midpoint.web.component.MultifunctionalButton; import com.evolveum.midpoint.web.component.data.ISelectableDataProvider; import com.evolveum.midpoint.web.component.data.SelectableBeanObjectDataProvider; -import com.evolveum.midpoint.web.component.data.column.ColumnUtils; import com.evolveum.midpoint.web.component.dialog.ChooseFocusTypeAndRelationDialogPanel; import com.evolveum.midpoint.web.component.dialog.ConfigureTaskConfirmationPanel; import com.evolveum.midpoint.web.component.form.MidpointForm; @@ -1497,7 +1496,7 @@ protected PrismObject getTask(AjaxRequestTarget target) { if (task == null) { return null; } - PrismObject recomputeTask = task.getClonedTaskObject(); + PrismObject recomputeTask = task.getRawTaskObjectClone(); TaskType recomputeTaskType = recomputeTask.asObjectable(); recomputeTaskType.getAssignment().add(ObjectTypeUtil.createAssignmentTo(SystemObjectsType.ARCHETYPE_RECOMPUTATION_TASK.value(), ObjectTypes.ARCHETYPE, getPrismContext())); return recomputeTask; diff --git a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/projector/focus/AssignmentTripleEvaluator.java b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/projector/focus/AssignmentTripleEvaluator.java index 1ccd7eee2f3..4c4f33689da 100644 --- a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/projector/focus/AssignmentTripleEvaluator.java +++ b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/projector/focus/AssignmentTripleEvaluator.java @@ -48,6 +48,8 @@ import org.jetbrains.annotations.NotNull; +import static com.evolveum.midpoint.util.DebugUtil.lazy; + /** * Evaluates all assignments and sorts them to triple: added, removed and "kept" assignments. * @@ -140,9 +142,8 @@ private Collection getVirtualAssignments() throws SchemaExceptio beans.prismContext, task, result); LOGGER.trace("Forced assignments: {}", forcedAssignments); - if (LOGGER.isTraceEnabled()) { - LOGGER.trace("Task for process: {}", task.getUpdatedOrClonedTaskObject().debugDump()); - } + LOGGER.trace("Task for process (operation result is not updated): {}", + lazy(() -> task.getRawTaskObjectClonedIfNecessary().debugDump())); Collection allTasksToRoot = task.getPathToRootTask(result); Collection taskAssignments = allTasksToRoot.stream() .filter(Task::hasAssignments) @@ -158,7 +159,7 @@ private Collection getVirtualAssignments() throws SchemaExceptio private AssignmentType createTaskAssignment(Task fromTask) { AssignmentType taskAssignment = new AssignmentType(beans.prismContext); ObjectReferenceType targetRef = new ObjectReferenceType(); - targetRef.asReferenceValue().setObject(fromTask.getUpdatedOrClonedTaskObject()); + targetRef.asReferenceValue().setObject(fromTask.getRawTaskObjectClonedIfNecessary()); taskAssignment.setTargetRef(targetRef); return taskAssignment; } diff --git a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/scripting/VariablesUtil.java b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/scripting/VariablesUtil.java index 5538e343ca0..1bd67f409ed 100644 --- a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/scripting/VariablesUtil.java +++ b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/scripting/VariablesUtil.java @@ -77,7 +77,8 @@ static VariablesMap initialPreparation(VariablesMap initialVariables, } private static void addProvidedVariables(VariablesMap resultingVariables, VariablesMap initialVariables, Task task) { - TypedValue taskValAndDef = new TypedValue<>(task.getUpdatedOrClonedTaskObject().asObjectable(), task.getUpdatedOrClonedTaskObject().getDefinition()); + PrismObject taskObject = task.getRawTaskObjectClonedIfNecessary(); + TypedValue taskValAndDef = new TypedValue<>(taskObject.asObjectable(), taskObject.getDefinition()); putImmutableValue(resultingVariables, ExpressionConstants.VAR_TASK, taskValAndDef); if (initialVariables != null) { initialVariables.forEach((key, value) -> putImmutableValue(resultingVariables, key, value)); diff --git a/model/model-test/src/main/java/com/evolveum/midpoint/model/test/AbstractModelIntegrationTest.java b/model/model-test/src/main/java/com/evolveum/midpoint/model/test/AbstractModelIntegrationTest.java index fa5f2e815b8..aec80c3f3f6 100644 --- a/model/model-test/src/main/java/com/evolveum/midpoint/model/test/AbstractModelIntegrationTest.java +++ b/model/model-test/src/main/java/com/evolveum/midpoint/model/test/AbstractModelIntegrationTest.java @@ -3279,7 +3279,7 @@ protected void displayTaskWithOperationStats(String message, PrismObject taskObject = task.getUpdatedOrClonedTaskObject(); + PrismObject taskObject = task.getRawTaskObjectClonedIfNecessary(); variables.put(ExpressionConstants.VAR_TASK, taskObject.asObjectable(), taskObject.getDefinition()); variables.put(ExpressionConstants.VAR_FILE, commandLineScriptExecutor.getOsSpecificFilePath(reportOutputFilePath), String.class); diff --git a/model/report-impl/src/main/java/com/evolveum/midpoint/report/impl/ReportTaskHandler.java b/model/report-impl/src/main/java/com/evolveum/midpoint/report/impl/ReportTaskHandler.java index 00ea94ca92e..d940c9dd57b 100644 --- a/model/report-impl/src/main/java/com/evolveum/midpoint/report/impl/ReportTaskHandler.java +++ b/model/report-impl/src/main/java/com/evolveum/midpoint/report/impl/ReportTaskHandler.java @@ -265,7 +265,7 @@ private void processPostReportScript(ReportType parentReport, String reportOutpu VariablesMap variables = new VariablesMap(); variables.put(ExpressionConstants.VAR_OBJECT, parentReport, parentReport.asPrismObject().getDefinition()); - PrismObject taskObject = task.getUpdatedOrClonedTaskObject(); + PrismObject taskObject = task.getRawTaskObjectClonedIfNecessary(); variables.put(ExpressionConstants.VAR_TASK, taskObject.asObjectable(), taskObject.getDefinition()); variables.put(ExpressionConstants.VAR_FILE, reportService.getCommandLineScriptExecutor().getOsSpecificFilePath(reportOutputFilePath), String.class); diff --git a/repo/task-api/src/main/java/com/evolveum/midpoint/task/api/Task.java b/repo/task-api/src/main/java/com/evolveum/midpoint/task/api/Task.java index 15ebd8f088e..62380a9a4a6 100644 --- a/repo/task-api/src/main/java/com/evolveum/midpoint/task/api/Task.java +++ b/repo/task-api/src/main/java/com/evolveum/midpoint/task/api/Task.java @@ -717,30 +717,25 @@ default List listSubtasks(OperationResult parentResult) throws S //region Task object as a whole /** - * Returns backing task prism object. - * - * *AVOID* use of this method if possible: - * - * - for regular tasks it has to update operation result in the prism object (might be costly) - * - for running tasks it provides a clone of the actual prism object (even more costly and leads to lost changes - * if the returned value is changed) + * Returns backing task prism object without updating with current operation result. + * If the task is running, a clone is returned. */ @NotNull - PrismObject getUpdatedOrClonedTaskObject(); + PrismObject getRawTaskObjectClonedIfNecessary(); /** - * Returns backing task prism object, provided that task is not running. - * Beware that the task operation result is updated (might be costly). - * @throws IllegalStateException if task is running + * Returns CLONE of backing task prism object without updating with current operation result. */ @NotNull - PrismObject getUpdatedTaskObject(); + PrismObject getRawTaskObjectClone(); /** - * Returns cloned task object. + * Returns backing task prism object UPDATED with current operation result. + * + * Assumes that task is not running. (Otherwise IllegalStateException is thrown.) */ @NotNull - PrismObject getClonedTaskObject(); + PrismObject getUpdatedTaskObject(); /** * Returns a reference to the task prism. diff --git a/repo/task-api/src/main/java/com/evolveum/midpoint/task/api/test/NullTaskImpl.java b/repo/task-api/src/main/java/com/evolveum/midpoint/task/api/test/NullTaskImpl.java index 847c9b2799d..8939f40b2e1 100644 --- a/repo/task-api/src/main/java/com/evolveum/midpoint/task/api/test/NullTaskImpl.java +++ b/repo/task-api/src/main/java/com/evolveum/midpoint/task/api/test/NullTaskImpl.java @@ -317,21 +317,14 @@ public void setProgressImmediate(Long progress, OperationResult parentResult) { throw new UnsupportedOperationException(); } - @NotNull - @Override - public PrismObject getUpdatedOrClonedTaskObject() { - throw new UnsupportedOperationException(); - } - @NotNull @Override public PrismObject getUpdatedTaskObject() { throw new UnsupportedOperationException(); } - @NotNull @Override - public PrismObject getClonedTaskObject() { + public @NotNull PrismObject getRawTaskObjectClone() { throw new UnsupportedOperationException(); } @@ -492,6 +485,11 @@ public List listPrerequisiteTasks(OperationResult parentResult) { throw new UnsupportedOperationException(); } + @Override + public @NotNull PrismObject getRawTaskObjectClonedIfNecessary() { + throw new UnsupportedOperationException(); + } + @Override public List getDependents() { throw new UnsupportedOperationException(); diff --git a/repo/task-quartz-impl/src/main/java/com/evolveum/midpoint/task/quartzimpl/LightweightTaskManager.java b/repo/task-quartz-impl/src/main/java/com/evolveum/midpoint/task/quartzimpl/LightweightTaskManager.java index 4473c401ce5..a2398d0236f 100644 --- a/repo/task-quartz-impl/src/main/java/com/evolveum/midpoint/task/quartzimpl/LightweightTaskManager.java +++ b/repo/task-quartz-impl/src/main/java/com/evolveum/midpoint/task/quartzimpl/LightweightTaskManager.java @@ -48,7 +48,7 @@ public Collection getTransientSubtasks(String identifier) { if (runningInstance != null) { List subtasks = new ArrayList<>(); for (RunningTaskQuartzImpl subtask : runningInstance.getLightweightAsynchronousSubtasks()) { - subtasks.add(subtask.cloneAsStaticTask()); + subtasks.add(subtask.cloneAsStaticTask()); // Beware, does not update operation result in task prism } return subtasks; } else { diff --git a/repo/task-quartz-impl/src/main/java/com/evolveum/midpoint/task/quartzimpl/TaskQuartzImpl.java b/repo/task-quartz-impl/src/main/java/com/evolveum/midpoint/task/quartzimpl/TaskQuartzImpl.java index 7116f96c00b..04aa2df4128 100644 --- a/repo/task-quartz-impl/src/main/java/com/evolveum/midpoint/task/quartzimpl/TaskQuartzImpl.java +++ b/repo/task-quartz-impl/src/main/java/com/evolveum/midpoint/task/quartzimpl/TaskQuartzImpl.java @@ -272,30 +272,32 @@ private boolean isLiveRunningInstance() { return this instanceof RunningTask; } - /** - * TODO TODO TODO (think out better name) - * Use with care. Never provide to outside world (beyond task manager). - */ - public PrismObject getLiveTaskObjectForNotRunningTasks() { + @NotNull + @Override + public PrismObject getRawTaskObjectClonedIfNecessary() { if (isLiveRunningInstance()) { - throw new UnsupportedOperationException("It is not possible to get live task prism object from the running task instance: " + this); + return getRawTaskObjectClone(); } else { return taskPrism; } } - // Use with utmost care! Never provide to outside world (beyond task manager) - public PrismObject getLiveTaskObject() { - return taskPrism; - } - @NotNull @Override - public PrismObject getUpdatedOrClonedTaskObject() { + public PrismObject getRawTaskObjectClone() { + synchronized (prismAccess) { + return taskPrism.clone(); + } + } + + + /** + * Returns the backing task prism object. Not supported for running task instances. + */ + public PrismObject getRawTaskObject() { if (isLiveRunningInstance()) { - return getClonedTaskObject(); + throw new IllegalStateException("Cannot get task object from live running task instance"); } else { - updateTaskPrismResult(taskPrism); return taskPrism; } } @@ -312,17 +314,7 @@ public PrismObject getUpdatedTaskObject() { } TaskQuartzImpl cloneAsStaticTask() { - return TaskQuartzImpl.createFromPrismObject(taskManager, getClonedTaskObject()); - } - - @NotNull - @Override - public PrismObject getClonedTaskObject() { - synchronized (prismAccess) { - PrismObject rv = taskPrism.clone(); - updateTaskPrismResult(rv); - return rv; - } + return TaskQuartzImpl.createFromPrismObject(taskManager, getRawTaskObjectClone()); } public boolean isRecreateQuartzTrigger() { diff --git a/repo/task-quartz-impl/src/main/java/com/evolveum/midpoint/task/quartzimpl/tasks/TaskPersister.java b/repo/task-quartz-impl/src/main/java/com/evolveum/midpoint/task/quartzimpl/tasks/TaskPersister.java index c0239a45be8..efceee81f78 100644 --- a/repo/task-quartz-impl/src/main/java/com/evolveum/midpoint/task/quartzimpl/tasks/TaskPersister.java +++ b/repo/task-quartz-impl/src/main/java/com/evolveum/midpoint/task/quartzimpl/tasks/TaskPersister.java @@ -96,7 +96,7 @@ private void persist(TaskQuartzImpl task, OperationResult result) { } try { - CryptoUtil.encryptValues(protector, task.getLiveTaskObjectForNotRunningTasks()); + CryptoUtil.encryptValues(protector, task.getRawTaskObject()); addTaskToRepositoryAndQuartz(task, null, result); } catch (ObjectAlreadyExistsException ex) { // This should not happen. If it does, it is a bug. It is OK to convert to a runtime exception diff --git a/repo/task-quartz-impl/src/main/java/com/evolveum/midpoint/task/quartzimpl/tasks/TaskRetriever.java b/repo/task-quartz-impl/src/main/java/com/evolveum/midpoint/task/quartzimpl/tasks/TaskRetriever.java index bbd40614ce4..ea33bc683c1 100644 --- a/repo/task-quartz-impl/src/main/java/com/evolveum/midpoint/task/quartzimpl/tasks/TaskRetriever.java +++ b/repo/task-quartz-impl/src/main/java/com/evolveum/midpoint/task/quartzimpl/tasks/TaskRetriever.java @@ -223,7 +223,7 @@ private List getSubtasks(Object task, OperationResult result) throws SchemaEx private void addSubtask(Object task, Object subtask) { TaskType subtaskBean; if (subtask instanceof TaskQuartzImpl) { - subtaskBean = ((TaskQuartzImpl) subtask).getLiveTaskObject().asObjectable(); + subtaskBean = ((TaskQuartzImpl) subtask).getRawTaskObjectClonedIfNecessary().asObjectable(); } else if (subtask instanceof PrismObject) { //noinspection unchecked subtaskBean = ((PrismObject) subtask).asObjectable(); @@ -336,7 +336,7 @@ private void addTransientTaskInformation(Object task, ClusterStatusInformation c } TaskType taskBean; if (task instanceof TaskQuartzImpl) { - taskBean = ((TaskQuartzImpl) task).getLiveTaskObjectForNotRunningTasks().asObjectable(); + taskBean = ((TaskQuartzImpl) task).getRawTaskObject().asObjectable(); } else if (task instanceof PrismObject) { //noinspection unchecked taskBean = ((PrismObject) task).asObjectable(); diff --git a/repo/task-quartz-impl/src/main/java/com/evolveum/midpoint/task/quartzimpl/tracing/TracingOutputCreator.java b/repo/task-quartz-impl/src/main/java/com/evolveum/midpoint/task/quartzimpl/tracing/TracingOutputCreator.java index 350a97e002c..8f92be7a121 100644 --- a/repo/task-quartz-impl/src/main/java/com/evolveum/midpoint/task/quartzimpl/tracing/TracingOutputCreator.java +++ b/repo/task-quartz-impl/src/main/java/com/evolveum/midpoint/task/quartzimpl/tracing/TracingOutputCreator.java @@ -86,7 +86,7 @@ private TracingEnvironmentType createTracingEnvironmentDescription(Task task, Tr selectedNodeInformation.setClustered(localNode.isClustered()); environment.setNodeRef(ObjectTypeUtil.createObjectRefWithFullObject(selectedNodeInformation, prismContext)); } - TaskType taskClone = task.getClonedTaskObject().asObjectable(); + TaskType taskClone = task.getRawTaskObjectClone().asObjectable(); // is it OK that we use not updated op. result? if (taskClone.getResult() != null) { taskClone.getResult().getPartialResults().clear(); } diff --git a/repo/task-quartz-impl/src/test/java/com/evolveum/midpoint/task/quartzimpl/MockParallelTaskHandler.java b/repo/task-quartz-impl/src/test/java/com/evolveum/midpoint/task/quartzimpl/MockParallelTaskHandler.java index 5cf3c0a2976..08182287f71 100644 --- a/repo/task-quartz-impl/src/test/java/com/evolveum/midpoint/task/quartzimpl/MockParallelTaskHandler.java +++ b/repo/task-quartz-impl/src/test/java/com/evolveum/midpoint/task/quartzimpl/MockParallelTaskHandler.java @@ -54,7 +54,7 @@ public static class MyLightweightTaskHandler implements LightweightTaskHandler { private boolean hasRun = false; private boolean hasExited = false; private final long duration; - private static final long STEP = 100; + private static final long STEP = 10; MyLightweightTaskHandler(Integer duration) { this.duration = duration != null ? duration : 86400L * 1000L * 365000L; // 1000 years @@ -72,8 +72,9 @@ public void run(RunningLightweightTask task) { //assertTrue("Subtask is not in Running LAT list of parent", isAmongRunningChildren(task, parentTask)); while (System.currentTimeMillis() < end && task.canRun()) { - // hoping to get ConcurrentModificationException when setting operation result here (MID-5113) - task.getLightweightTaskParent().getUpdatedOrClonedTaskObject(); + // Check for some concurrency issues - although not related to the operation result, because + // it is inherently not thread-safe. + task.getLightweightTaskParent().getRawTaskObjectClonedIfNecessary(); IterationItemInformation info = new IterationItemInformation("o1", null, UserType.COMPLEX_TYPE, "oid1"); Operation op = task.recordIterativeOperationStart(info); try { @@ -88,6 +89,9 @@ public void run(RunningLightweightTask task) { op.failed(e); break; } + OperationResult result = task.getResult(); + result.createSubresult("test"); + result.summarize(); } hasExited = true; } diff --git a/repo/task-quartz-impl/src/test/java/com/evolveum/midpoint/task/quartzimpl/TestTaskManagerBasic.java b/repo/task-quartz-impl/src/test/java/com/evolveum/midpoint/task/quartzimpl/TestTaskManagerBasic.java index b3f4b925689..c30ac30a6bd 100644 --- a/repo/task-quartz-impl/src/test/java/com/evolveum/midpoint/task/quartzimpl/TestTaskManagerBasic.java +++ b/repo/task-quartz-impl/src/test/java/com/evolveum/midpoint/task/quartzimpl/TestTaskManagerBasic.java @@ -27,6 +27,8 @@ import com.evolveum.midpoint.schema.util.task.TaskTreeUtil; import com.evolveum.midpoint.task.api.RunningLightweightTask; +import com.evolveum.midpoint.util.exception.CommonException; + import org.jetbrains.annotations.NotNull; import org.quartz.JobExecutionContext; import org.quartz.JobKey; @@ -52,7 +54,6 @@ import com.evolveum.midpoint.schema.constants.SchemaConstants; import com.evolveum.midpoint.schema.result.OperationResult; import com.evolveum.midpoint.schema.result.OperationResultStatus; -import com.evolveum.midpoint.task.api.RunningTask; import com.evolveum.midpoint.task.api.Task; import com.evolveum.midpoint.task.api.TaskConstants; import com.evolveum.midpoint.task.quartzimpl.cluster.ClusterManager; @@ -769,6 +770,9 @@ public void test250TaskWithThreads() throws Exception { when(); add(TASK_WITH_THREADS, result); + + checkTreadSafety(TASK_WITH_THREADS.oid, 1000L, result); + waitUntilDone(TASK_WITH_THREADS.oid, result, 15000, 100); waitForTaskClose(TASK_WITH_THREADS.oid, result, 15000, 100); @@ -787,6 +791,40 @@ public void test250TaskWithThreads() throws Exception { } } + /** + * A simple test for MID-6910. + */ + @SuppressWarnings("SameParameterValue") + private void checkTreadSafety(String oid, long duration, OperationResult result) + throws CommonException, InterruptedException { + + PrismObject retrievedTask; + long start = System.currentTimeMillis(); + for (;;) { + Collection> options = schemaService.getOperationOptionsBuilder() + .item(TaskType.F_SUBTASK_REF).retrieve() + .build(); + retrievedTask = taskManager.getObject(TaskType.class, oid, options, result); + if (!retrievedTask.asObjectable().getSubtaskRef().isEmpty()) { + break; + } + if (System.currentTimeMillis() - start > 3000) { + throw new AssertionError("Timed out waiting for the task to create subtasks"); + } + //noinspection BusyWait + Thread.sleep(200); + } + + display("Subtasks: " + retrievedTask.asObjectable().getSubtaskRef().size()); + long startCloning = System.currentTimeMillis(); + int cloneIterations = 0; + while (System.currentTimeMillis() - startCloning < duration) { + retrievedTask.clone(); + cloneIterations++; + } + display("Clone iterations done: " + cloneIterations); + } + @SuppressWarnings("SameParameterValue") private void waitUntilDone(String taskOid, OperationResult result, int duration, int checkInterval) throws SchemaException, ObjectNotFoundException, InterruptedException {