Skip to content

Commit

Permalink
tui: fix an obscure freezing issue
Browse files Browse the repository at this point in the history
* 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.
  • Loading branch information
oliver-sanders committed Oct 3, 2023
1 parent e491bf3 commit 2b969af
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 29 deletions.
48 changes: 37 additions & 11 deletions cylc/flow/tui/data.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -232,26 +251,33 @@ 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)

Check warning on line 262 in cylc/flow/tui/data.py

View check run for this annotation

Codecov / codecov/patch

cylc/flow/tui/data.py#L262

Added line #L262 was not covered by tests


def online_mutate(client, mutation, selection):
def online_mutate(mutation, selection):
"""Issue a mutation over a network interface."""
context = extract_context(selection)
variables = context_to_variables(context)

try:
client = get_client(variables['workflow'])

Check warning on line 271 in cylc/flow/tui/data.py

View check run for this annotation

Codecov / codecov/patch

cylc/flow/tui/data.py#L270-L271

Added lines #L270 - L271 were not covered by tests
except WorkflowStopped:
raise Exception(

Check warning on line 273 in cylc/flow/tui/data.py

View check run for this annotation

Codecov / codecov/patch

cylc/flow/tui/data.py#L273

Added line #L273 was not covered by tests
f'Cannot peform command {mutation} on a stopped workflow'
)
except (ClientError, ClientTimeout) as exc:
raise Exception(

Check warning on line 277 in cylc/flow/tui/data.py

View check run for this annotation

Codecov / codecov/patch

cylc/flow/tui/data.py#L276-L277

Added lines #L276 - L277 were not covered by tests
f'Error connecting to workflow: {exc}'
)

request_string = generate_mutation(mutation, variables)
client(
'graphql',
Expand Down
34 changes: 16 additions & 18 deletions cylc/flow/tui/overlay.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -61,7 +54,6 @@
TUI
)
from cylc.flow.tui.data import (
extract_context,
list_mutations,
mutate,
)
Expand Down Expand Up @@ -280,20 +272,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:

Check warning on line 291 in cylc/flow/tui/overlay.py

View check run for this annotation

Codecov / codecov/patch

cylc/flow/tui/overlay.py#L290-L291

Added lines #L290 - L291 were not covered by tests
app.open_overlay(partial(error, text=str(exc)))
else:
app.close_topmost()
Expand Down Expand Up @@ -324,7 +319,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,
)
]
)
)
Expand Down

0 comments on commit 2b969af

Please sign in to comment.