From 6f3f1701c1106ecd779b11b08ad4293186678da0 Mon Sep 17 00:00:00 2001
From: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
Date: Thu, 28 Sep 2023 16:55:10 +0100
Subject: [PATCH] tui: fix an obscure freezing issue

* Fixes an obscure issue where the applications main loop becomes
  suspended as the result of attempting to register an `on_press`
  callback which takes as an argument a Cylc client.
* Even though this callback is not actually called at this point,
  just passing the reference to it appears to be enough to cause
  thread deadlocking because Cylc clients are async based and
  not thread safe.
---
 cylc/flow/tui/data.py    | 34 ++++++++++++++++++++++++----------
 cylc/flow/tui/overlay.py | 34 ++++++++++++++++------------------
 2 files changed, 40 insertions(+), 28 deletions(-)

diff --git a/cylc/flow/tui/data.py b/cylc/flow/tui/data.py
index 3a3ed4ee378..4d00753cf3f 100644
--- a/cylc/flow/tui/data.py
+++ b/cylc/flow/tui/data.py
@@ -17,7 +17,12 @@
 from functools import partial
 from subprocess import Popen, PIPE
 
-from cylc.flow.exceptions import ClientError
+from cylc.flow.exceptions import (
+    ClientError,
+    ClientTimeout,
+    WorkflowStopped,
+)
+from cylc.flow.network.client_factory import get_client
 from cylc.flow.id import Tokens
 from cylc.flow.tui.util import (
     extract_context
@@ -187,11 +192,25 @@ def generate_mutation(mutation, arguments):
     '''
 
 
-def list_mutations(client, selection):
+def list_mutations(selection, is_running=True):
+    """List mutations relevant to the provided selection.
+
+    Args:
+        selection:
+            The user selection.
+        is_running:
+            If False, then mutations which require the scheduler to be
+            running will be omitted.
+
+            Note, this is only relevant for workflow nodes because if a
+            workflow is stopped, then any tasks within it will be removed
+            anyway.
+
+    """
     context = extract_context(selection)
     selection_type = list(context)[-1]
     ret = []
-    if client:
+    if is_running:
         # add the online mutations
         ret.extend(MUTATIONS.get(selection_type, []))
     # add the offline mutations
@@ -232,20 +251,15 @@ def context_to_variables(context):
     return variables
 
 
-def mutate(client, mutation, selection):
+def mutate(mutation, selection):
     if mutation in {
         _mutation
         for section in OFFLINE_MUTATIONS.values()
         for _mutation in section
     }:
         offline_mutate(mutation, selection)
-    elif client:
-        online_mutate(client, mutation, selection)
     else:
-        raise Exception(
-            f'Cannot peform command {mutation} on a stopped workflow'
-            ' or invalid command.'
-        )
+        online_mutate(mutation, selection)
 
 
 def online_mutate(mutation, selection):
diff --git a/cylc/flow/tui/overlay.py b/cylc/flow/tui/overlay.py
index f4d5ca02f94..87b544b0604 100644
--- a/cylc/flow/tui/overlay.py
+++ b/cylc/flow/tui/overlay.py
@@ -37,20 +37,13 @@
 
 """
 
-from contextlib import suppress
 from functools import partial
 import re
 import sys
 
 import urwid
 
-from cylc.flow.exceptions import (
-    ClientError,
-    ClientTimeout,
-    WorkflowStopped,
-)
 from cylc.flow.id import Tokens
-from cylc.flow.network.client_factory import get_client
 from cylc.flow.task_state import (
     TASK_STATUSES_ORDERED,
     TASK_STATUS_WAITING
@@ -61,7 +54,6 @@
     TUI
 )
 from cylc.flow.tui.data import (
-    extract_context,
     list_mutations,
     mutate,
 )
@@ -296,20 +288,23 @@ def context(app):
     """An overlay for context menus."""
     value = app.tree_walker.get_focus()[0].get_node().get_value()
     selection = [value['id_']]  # single selection ATM
-    context = extract_context(selection)
 
-    client = None
-    if 'workflow' in context:
-        w_id = context['workflow'][0]
-        with suppress(WorkflowStopped, ClientError, ClientTimeout):
-            client = get_client(w_id)
+    is_running = True
+    if (
+        value['type_'] == 'workflow'
+        and value['data']['status'] not in {'running', 'paused'}
+    ):
+        # this is a stopped workflow
+        # => don't display mutations only valid for a running workflow
+        is_running = False
 
     def _mutate(mutation, _):
-        nonlocal app, client
+        nonlocal app, selection
+
         app.open_overlay(partial(progress, text='Running Command'))
         try:
-            mutate(client, mutation, selection)
-        except ClientError as exc:
+            mutate(mutation, selection)
+        except Exception as exc:
             app.open_overlay(partial(error, text=str(exc)))
         else:
             app.close_topmost()
@@ -334,7 +329,10 @@ def _mutate(mutation, _):
                     mutation,
                     on_press=partial(_mutate, mutation)
                 )
-                for mutation in list_mutations(client, selection)
+                for mutation in list_mutations(
+                    selection,
+                    is_running,
+                )
             ]
         )
     )