Lock TestWorkflowStoreImpl when listing workflows (#890)

This is the only path to the backing histories map that isn't locked. When running a test containing multiple child workflows in parallel where one child workflow attempts to call listWorkflows I'm able to consistently reproduce a concurrent modification exception.

Despite being a change to locking, this change is rather low risk. The lock is reentrant and there are no other locks acquired while holding this lock, so there's no chance of deadlock. In addition, this codepath isn't use internally at all. It exists only to implement ListOpenWorkflowExecutions and ListClosedWorkflowExecutions on the client, allowing test code to make these RPCs. This is likely why it hasn't been caught before.

Author: natemort

src/main/java/com/uber/cadence/internal/testservice/TestWorkflowStoreImpl.java

@@ -407,51 +407,66 @@ public void getDiagnostics(StringBuilder result) {
   @Override
   public List<WorkflowExecutionInfo> listWorkflows(
       WorkflowState state, Optional<String> filterWorkflowId) {
-    List<WorkflowExecutionInfo> result = new ArrayList<>();
-    for (Entry<ExecutionId, HistoryStore> entry : this.histories.entrySet()) {
-      if (state == WorkflowState.OPEN) {
-        if (entry.getValue().isCompleted()) {
-          continue;
-        }
-        ExecutionId executionId = entry.getKey();
-        String workflowId = executionId.getWorkflowId().getWorkflowId();
-        if (filterWorkflowId.isPresent() && !workflowId.equals(filterWorkflowId.get())) {
-          continue;
-        }
-        List<HistoryEvent> history = entry.getValue().getHistory();
-        WorkflowExecutionInfo info =
-            new WorkflowExecutionInfo()
-                .setExecution(executionId.getExecution())
-                .setHistoryLength(history.size())
-                .setStartTime(history.get(0).getTimestamp())
-                .setIsCron(
-                    history.get(0).getWorkflowExecutionStartedEventAttributes().isSetCronSchedule())
-                .setType(
-                    history.get(0).getWorkflowExecutionStartedEventAttributes().getWorkflowType());
-        result.add(info);
-      } else {
-        if (!entry.getValue().isCompleted()) {
-          continue;
-        }
-        ExecutionId executionId = entry.getKey();
-        String workflowId = executionId.getWorkflowId().getWorkflowId();
-        if (filterWorkflowId.isPresent() && !workflowId.equals(filterWorkflowId.get())) {
-          continue;
+    lock.lock();
+    List<WorkflowExecutionInfo> result;
+    try {
+      result = new ArrayList<>();
+      for (Entry<ExecutionId, HistoryStore> entry : this.histories.entrySet()) {
+        if (state == WorkflowState.OPEN) {
+          if (entry.getValue().isCompleted()) {
+            continue;
+          }
+          ExecutionId executionId = entry.getKey();
+          String workflowId = executionId.getWorkflowId().getWorkflowId();
+          if (filterWorkflowId.isPresent() && !workflowId.equals(filterWorkflowId.get())) {
+            continue;
+          }
+          List<HistoryEvent> history = entry.getValue().getHistory();
+          WorkflowExecutionInfo info =
+              new WorkflowExecutionInfo()
+                  .setExecution(executionId.getExecution())
+                  .setHistoryLength(history.size())
+                  .setStartTime(history.get(0).getTimestamp())
+                  .setIsCron(
+                      history
+                          .get(0)
+                          .getWorkflowExecutionStartedEventAttributes()
+                          .isSetCronSchedule())
+                  .setType(
+                      history
+                          .get(0)
+                          .getWorkflowExecutionStartedEventAttributes()
+                          .getWorkflowType());
+          result.add(info);
+        } else {
+          if (!entry.getValue().isCompleted()) {
+            continue;
+          }
+          ExecutionId executionId = entry.getKey();
+          String workflowId = executionId.getWorkflowId().getWorkflowId();
+          if (filterWorkflowId.isPresent() && !workflowId.equals(filterWorkflowId.get())) {
+            continue;
+          }
+          List<HistoryEvent> history = entry.getValue().getHistory();
+          WorkflowExecutionInfo info =
+              new WorkflowExecutionInfo()
+                  .setExecution(executionId.getExecution())
+                  .setHistoryLength(history.size())
+                  .setStartTime(history.get(0).getTimestamp())
+                  .setIsCron(
+                      history
+                          .get(0)
+                          .getWorkflowExecutionStartedEventAttributes()
+                          .isSetCronSchedule())
+                  .setType(
+                      history.get(0).getWorkflowExecutionStartedEventAttributes().getWorkflowType())
+                  .setCloseStatus(
+                      WorkflowExecutionUtils.getCloseStatus(history.get(history.size() - 1)));
+          result.add(info);
         }
-        List<HistoryEvent> history = entry.getValue().getHistory();
-        WorkflowExecutionInfo info =
-            new WorkflowExecutionInfo()
-                .setExecution(executionId.getExecution())
-                .setHistoryLength(history.size())
-                .setStartTime(history.get(0).getTimestamp())
-                .setIsCron(
-                    history.get(0).getWorkflowExecutionStartedEventAttributes().isSetCronSchedule())
-                .setType(
-                    history.get(0).getWorkflowExecutionStartedEventAttributes().getWorkflowType())
-                .setCloseStatus(
-                    WorkflowExecutionUtils.getCloseStatus(history.get(history.size() - 1)));
-        result.add(info);
       }
+    } finally {
+      lock.unlock();
     }
     return result;
   }