From 12f11a357d4ee8859a408357c57b1cad854ed841 Mon Sep 17 00:00:00 2001
From: Ali Reza Zamani Zadeh Najari <anajari@linkedin.com>
Date: Wed, 12 Feb 2020 11:44:53 -0800
Subject: [PATCH] Fix ConcurrentModification exception in Workflow Garbage
 Collection (#741)

In workflow Garbage collection, there is possibility that we encounter
ConcurrentMod exception while looping through the workflow contexts.
This commit fixes this issue by adding a try-catch.
---
 .../java/org/apache/helix/task/TaskUtil.java  | 37 ++++++++++++++-----
 .../TestWorkflowContextWithoutConfig.java     |  2 +-
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/helix-core/src/main/java/org/apache/helix/task/TaskUtil.java b/helix-core/src/main/java/org/apache/helix/task/TaskUtil.java
index 6b36db477e..5ae2f07eef 100644
--- a/helix-core/src/main/java/org/apache/helix/task/TaskUtil.java
+++ b/helix-core/src/main/java/org/apache/helix/task/TaskUtil.java
@@ -1043,23 +1043,40 @@ public static void purgeExpiredJobs(String workflow, WorkflowConfig workflowConf
    * @param dataProvider
    * @param manager
    */
-  public static void workflowGarbageCollection(WorkflowControllerDataProvider dataProvider,
+  public static void workflowGarbageCollection(final WorkflowControllerDataProvider dataProvider,
       final HelixManager manager) {
     // Garbage collections for conditions where workflow context exists but config is missing.
-    Map<String, ZNRecord> contexts = dataProvider.getContexts();
-    HelixDataAccessor accessor = manager.getHelixDataAccessor();
-    HelixPropertyStore<ZNRecord> propertyStore = manager.getHelixPropertyStore();
 
+    Set<String> existingContexts;
+    /*
+     * Here try-catch is used to avoid concurrent modification exception while doing deep copy.
+     * Map.keySet() can produce concurrent modification exception.
+     * Reason: If the map is modified while an iteration over the set is in progress, concurrent
+     * modification exception will be thrown.
+     */
+    try {
+      existingContexts = new HashSet<>(dataProvider.getContexts().keySet());
+    } catch (Exception e) {
+      LOG.warn(
+          "Exception occurred while creating a list of all workflow/job context names!",
+          e);
+      return;
+    }
+
+    // toBeDeletedWorkflows is a set that contains the name of the workflows that their contexts
+    // should be deleted.
     Set<String> toBeDeletedWorkflows = new HashSet<>();
-    for (Map.Entry<String, ZNRecord> entry : contexts.entrySet()) {
-      if (entry.getValue() != null
-          && entry.getValue().getId().equals(TaskUtil.WORKFLOW_CONTEXT_KW)) {
-        if (dataProvider.getWorkflowConfig(entry.getKey()) == null) {
-          toBeDeletedWorkflows.add(entry.getKey());
-        }
+    for (String entry : existingContexts) {
+      WorkflowConfig cfg = dataProvider.getWorkflowConfig(entry);
+      WorkflowContext ctx = dataProvider.getWorkflowContext(entry);
+      if (ctx != null && ctx.getId().equals(TaskUtil.WORKFLOW_CONTEXT_KW) && cfg == null) {
+        toBeDeletedWorkflows.add(entry);
       }
     }
 
+    HelixDataAccessor accessor = manager.getHelixDataAccessor();
+    HelixPropertyStore<ZNRecord> propertyStore = manager.getHelixPropertyStore();
+
     for (String workflowName : toBeDeletedWorkflows) {
       LOG.warn(String.format(
           "WorkflowContext exists for workflow %s. However, Workflow Config is missing! Deleting the WorkflowConfig and IdealState!!",
diff --git a/helix-core/src/test/java/org/apache/helix/integration/task/TestWorkflowContextWithoutConfig.java b/helix-core/src/test/java/org/apache/helix/integration/task/TestWorkflowContextWithoutConfig.java
index b7d6016da0..d1102e1a51 100644
--- a/helix-core/src/test/java/org/apache/helix/integration/task/TestWorkflowContextWithoutConfig.java
+++ b/helix-core/src/test/java/org/apache/helix/integration/task/TestWorkflowContextWithoutConfig.java
@@ -170,7 +170,7 @@ record = _manager.getHelixDataAccessor().getBaseDataAccessor().get(workflowConte
     Assert.assertTrue(contextDeleted);
   }
 
-  Workflow.Builder createSimpleWorkflowBuilder(String workflowName) {
+  private Workflow.Builder createSimpleWorkflowBuilder(String workflowName) {
     final long expiryTime = 5000L;
     Workflow.Builder builder = new Workflow.Builder(workflowName);