Yair Zaslavsky has uploaded a new change for review. Change subject: core: AsyncTask fixes ......................................................................
core: AsyncTask fixes This patch includes - 1. Removal of static lock that was used for synchronizing access to the entity id -> EntityMultiAsyncTask static map 2. Change The type of the map from HashMap to ConcurrentHashMap (as result of 1) 3. Improve locking at EntityMultiAsyncTask (as a result of 2) Change-Id: I55a49ae5d655c2105c5840decec81ae712e40c11 Signed-off-by: Yair Zaslavsky <[email protected]> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/EntityAsyncTask.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/EntityMultiAsyncTasks.java 2 files changed, 69 insertions(+), 73 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/13/8813/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/EntityAsyncTask.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/EntityAsyncTask.java index 241bfe0..7c39258 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/EntityAsyncTask.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/EntityAsyncTask.java @@ -1,7 +1,7 @@ package org.ovirt.engine.core.bll; -import java.util.HashMap; -import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; import java.util.concurrent.atomic.AtomicInteger; import org.ovirt.engine.core.bll.context.CommandContext; @@ -23,10 +23,9 @@ * other tasks regarding the relevant entity have already ended. */ public class EntityAsyncTask extends SPMAsyncTask { - private static final Object _lockObject = new Object(); - private static final Map<Object, EntityMultiAsyncTasks> _multiTasksByEntities = - new HashMap<Object, EntityMultiAsyncTasks>(); + private static final ConcurrentMap<Object, EntityMultiAsyncTasks> _multiTasksByEntities = + new ConcurrentHashMap<Object, EntityMultiAsyncTasks>(); private static final AtomicInteger _endActionsInProgress = new AtomicInteger(0); @@ -35,14 +34,7 @@ } private static EntityMultiAsyncTasks GetEntityMultiAsyncTasksByContainerId(Object containerID) { - EntityMultiAsyncTasks entityInfo = null; - synchronized (_lockObject) { - if (_multiTasksByEntities.containsKey(containerID) && _multiTasksByEntities.get(containerID) != null) { - entityInfo = _multiTasksByEntities.get(containerID); - } - } - - return entityInfo; + return _multiTasksByEntities.get(containerID); } private EntityMultiAsyncTasks GetEntityMultiAsyncTasks() { @@ -51,22 +43,30 @@ public EntityAsyncTask(AsyncTaskParameters parameters) { super(parameters); - synchronized (_lockObject) { - if (!_multiTasksByEntities.containsKey(getContainerId())) { + EntityMultiAsyncTasks oldValue = _multiTasksByEntities.putIfAbsent(getContainerId(), new EntityMultiAsyncTasks(getContainerId())); + if (oldValue == null) { log.infoFormat("EntityAsyncTask::Adding EntityMultiAsyncTasks object for entity '{0}'", getContainerId()); - _multiTasksByEntities.put(getContainerId(), new EntityMultiAsyncTasks(getContainerId())); } - } - EntityMultiAsyncTasks entityInfo = GetEntityMultiAsyncTasks(); - entityInfo.AttachTask(this); + entityInfo.writeLock(); + try { + entityInfo.AttachTask(this); + } finally { + entityInfo.writeUnlock(); + } } @Override protected void ConcreteStartPollingTask() { EntityMultiAsyncTasks entityInfo = GetEntityMultiAsyncTasks(); - entityInfo.StartPollingTask(getTaskID()); + entityInfo.readLock(); + try { + entityInfo.StartPollingTask(getTaskID()); + } + finally { + entityInfo.readUnlock(); + } } @Override @@ -84,9 +84,19 @@ getTaskID()); ClearAsyncTask(); + return; } + entityInfo.readLock(); + try { + endActionForTasks(entityInfo); + } + finally { + entityInfo.readUnlock(); + } + } - else if (entityInfo.ShouldEndAction()) { + private void endActionForTasks(EntityMultiAsyncTasks entityInfo) { + if (entityInfo.ShouldEndAction()) { log.infoFormat( "EntityAsyncTask::EndActionIfNecessary: All tasks of entity '{0}' has ended -> executing 'EndAction'", getContainerId()); @@ -192,9 +202,10 @@ ExecutionHandler.endTaskJob(context, vdcReturnValue.getSucceeded() && isTaskGroupSuccess); } - entityInfo.ClearTasks(); - - synchronized (_lockObject) { + entityInfo.readLock(); + try { + //synchronized (_lockObject) { + entityInfo.ClearTasks(); if (entityInfo.getAllCleared()) { log.infoFormat( "EntityAsyncTask::HandleEndActionResult [within thread]: Removing EntityMultiAsyncTasks object for entity '{0}'", @@ -204,6 +215,9 @@ entityInfo.resetActionTypeIfNecessary(); entityInfo.StartPollingNextTask(); } + //} + } finally { + entityInfo.readUnlock(); } } } diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/EntityMultiAsyncTasks.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/EntityMultiAsyncTasks.java index e007fcc..8b4da4c 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/EntityMultiAsyncTasks.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/EntityMultiAsyncTasks.java @@ -1,6 +1,6 @@ package org.ovirt.engine.core.bll; -import java.util.List; +import java.util.concurrent.locks.ReentrantReadWriteLock; import org.ovirt.engine.core.common.action.VdcActionType; import org.ovirt.engine.core.common.asynctasks.EndedTaskInfo; @@ -11,9 +11,26 @@ public class EntityMultiAsyncTasks { private VdcActionType privateActionType = VdcActionType.forValue(0); + private ReentrantReadWriteLock lock = new ReentrantReadWriteLock(); public VdcActionType getActionType() { return privateActionType; + } + + public void readLock() { + lock.readLock().lock(); + } + + public void readUnlock() { + lock.readLock().unlock(); + } + + public void writeLock() { + lock.writeLock().lock(); + } + + public void writeUnlock() { + lock.writeLock().unlock(); } public void setActionType(VdcActionType value) { @@ -37,14 +54,12 @@ } public void AttachTask(EntityAsyncTask asyncTask) { - synchronized (_listTasks) { if (!_listTasks.containsKey(asyncTask.getTaskID())) { log.infoFormat("EntityMultiAsyncTasks::AttachTask: Attaching task '{0}' to entity '{1}'.", asyncTask.getTaskID(), getContainerId()); _listTasks.put(asyncTask.getTaskID(), asyncTask); } - } } private java.util.ArrayList<EntityAsyncTask> GetCurrentActionTypeTasks() { @@ -64,36 +79,30 @@ } public boolean ShouldEndAction() { - synchronized (_listTasks) { - java.util.ArrayList<EntityAsyncTask> CurrentActionTypeTasks = GetCurrentActionTypeTasks(); - - for (EntityAsyncTask task : CurrentActionTypeTasks) { + for (EntityAsyncTask task : GetCurrentActionTypeTasks()) { if (task.getState() != AsyncTaskState.Ended) { return false; } } - } - return true; } public void MarkAllWithAttemptingEndAction() { - synchronized (_listTasks) { - java.util.ArrayList<EntityAsyncTask> CurrentActionTypeTasks = GetCurrentActionTypeTasks(); - - for (EntityAsyncTask task : CurrentActionTypeTasks) { + for (EntityAsyncTask task : GetCurrentActionTypeTasks()) { task.setState(AsyncTaskState.AttemptingEndAction); } - } } public EndedTasksInfo getEndedTasksInfo() { EndedTasksInfo endedTasksInfo = new EndedTasksInfo(); java.util.ArrayList<EndedTaskInfo> endedTaskInfoList = new java.util.ArrayList<EndedTaskInfo>(); - - synchronized (_listTasks) { - java.util.ArrayList<EntityAsyncTask> CurrentActionTypeTasks = GetCurrentActionTypeTasks(); - + java.util.ArrayList<EntityAsyncTask> CurrentActionTypeTasks = null; + readLock(); + try { + CurrentActionTypeTasks = GetCurrentActionTypeTasks(); + } finally { + readUnlock(); + } for (EntityAsyncTask task : CurrentActionTypeTasks) { task.setLastStatusAccessTime(); EndedTaskInfo tempVar = new EndedTaskInfo(); @@ -103,29 +112,17 @@ } endedTasksInfo.setTasksInfo(endedTaskInfoList); - } - return endedTasksInfo; } public int getTasksCountCurrentActionType() { - int returnValue = 0; - synchronized (_listTasks) { - java.util.ArrayList<EntityAsyncTask> CurrentActionTypeTasks = GetCurrentActionTypeTasks(); - returnValue = CurrentActionTypeTasks.size(); - } - - return returnValue; + return GetCurrentActionTypeTasks().size(); } public void Repoll() { - synchronized (_listTasks) { - java.util.ArrayList<EntityAsyncTask> CurrentActionTypeTasks = GetCurrentActionTypeTasks(); - - for (EntityAsyncTask task : CurrentActionTypeTasks) { + for (EntityAsyncTask task : GetCurrentActionTypeTasks()) { task.setState(AsyncTaskState.Ended); } - } } /** @@ -134,35 +131,26 @@ */ protected void resetActionTypeIfNecessary() { boolean allCleared = true; - synchronized (_listTasks) { - List<EntityAsyncTask> CurrentActionTypeTasks = GetCurrentActionTypeTasks(); - - for (EntityAsyncTask task : CurrentActionTypeTasks) { + for (EntityAsyncTask task : GetCurrentActionTypeTasks()) { allCleared = allCleared && taskWasCleared(task); } if (allCleared) { setActionType(VdcActionType.Unknown); } - } } public void ClearTasks() { - synchronized (_listTasks) { - java.util.ArrayList<EntityAsyncTask> CurrentActionTypeTasks = GetCurrentActionTypeTasks(); - - for (EntityAsyncTask task : CurrentActionTypeTasks) { + for (EntityAsyncTask task : GetCurrentActionTypeTasks()) { task.ClearAsyncTask(); } StartPollingNextTask(); - } } // call this method after ending action and clearing tasks of current // ActionType. protected void StartPollingNextTask() { - synchronized (_listTasks) { for (EntityAsyncTask task : _listTasks.values()) { if (task.getState() == AsyncTaskState.WaitForPoll && task.getParameters() != null && task.getParameters().getDbAsyncTask() != null) { @@ -177,11 +165,9 @@ break; } } - } } public void StartPollingTask(Guid TaskID) { - synchronized (_listTasks) { if (_listTasks.containsKey(TaskID) && _listTasks.get(TaskID).getParameters() != null && _listTasks.get(TaskID).getParameters().getDbAsyncTask() != null) { if (getActionType() == VdcActionType.Unknown) { @@ -226,18 +212,14 @@ "EntityMultiAsyncTasks::StartPollingTask: For some reason, task '{0}' has no parameters or no DB task - we don't know its action type", TaskID); } - } } public boolean getAllCleared() { - synchronized (_listTasks) { for (EntityAsyncTask task : _listTasks.values()) { if (!taskWasCleared(task)) { return false; } } - } - return true; } -- To view, visit http://gerrit.ovirt.org/8813 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I55a49ae5d655c2105c5840decec81ae712e40c11 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
