Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tui: prevent hang on shutdown #6178

Merged
merged 1 commit into from
Jul 3, 2024
Merged

Conversation

oliver-sanders
Copy link
Member

It has been reported that Tui can hang on shutdown.

The easiest way to reproduce this is to press "q" as soon as starting Tui. Sometimes this can cause it to hang.

$ cylc vip <workflow>
$ cylc tui <workflow>
q

This PR adds a timeout on the updater shutdown to prevent any hanging operation stopping Tui from exiting.

This isn't really the "right" way to solve the problem, getting the updater to exit cleanly in all circumstances would be nicer. However, it's a reasonable fail safe and since the updater is a read-only process (i.e. the only "resources" it's managing are Cylc clients which have timeouts built in) there are no ill effects of terminating it.

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Not reasonably testable.
  • CHANGES.md entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

* Add a timeout on the updater shutdown to prevent any hanging operation
  stopping Tui from exiting.
@oliver-sanders oliver-sanders added this to the 8.3.1 milestone Jun 28, 2024
@oliver-sanders oliver-sanders self-assigned this Jun 28, 2024
@oliver-sanders oliver-sanders changed the title tui: tui: prevent hang on shutdown Jun 28, 2024
@oliver-sanders oliver-sanders requested a review from wxtim June 28, 2024 09:42
@oliver-sanders oliver-sanders added bug Something is wrong :( small labels Jun 28, 2024
Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have struggled to repeat the original bug, but this looks ok.

@oliver-sanders
Copy link
Member Author

@wxtim, the original bug isn't reproducing as easily as it originally did. To simulate this bug, try changing break to continue in this code:

async def run(self, filters):
"""Start the updater in an existing asyncio.loop.
The tests call this within the same process.
"""
with suppress_logging():
self._update_filters(filters)
while True:
ret = await self._update()
if ret == self.SIGNAL_TERMINATE:
break
self.update_queue.put(ret)

That will prevent the updater from shutting down. Before this commit, Tui should hang indefinitely, after this PR it should exit after 4 seconds (due to the timeout).

@wxtim
Copy link
Member

wxtim commented Jul 3, 2024

try changing break to continue in this code

Looks like I can now repeat the problem. Pleasingly this branch fixes it!

@oliver-sanders oliver-sanders merged commit 4ec556a into cylc:8.3.x Jul 3, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :( small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants