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

Deadlock when closing app, associated with Footer.recompose and focus events #4677

Closed
slawek-es opened this issue Jun 26, 2024 · 5 comments
Closed

Comments

@slawek-es
Copy link

A deadlock similar to the one described in #4643 occurs in a simple test case where the application is closed very soon after it gets initialized. The supposed fix for #4643 does not work for this case, and the deadlock is still reproducible with textual version 0.70.0.

The issue seems to again be a bad interaction between the Footer.recompose call run as the response to the bindings_updated signal and the procedure of closing the entire application. The Footer.recompose enqueues an AwaitRemove callback while the _dom_lock is already held by _close_all, and closing the Footer requires completing the AwaitRemove future, leading to a deadlock. The issue seems to be triggered by set_focus calls which we do from a background task.

Minimal Reproduction Environment

Run pytest editshare_system_upgrade/textual_ui/repro_app.py to see the TimeoutError:

import asyncio

from textual import work
from textual.app import App, ComposeResult
from textual.widgets import Header, Footer, Input
from textual.screen import Screen


class ReproScreen(Screen[None]):
    def __init__(self) -> None:
        super().__init__()
        self._input = Input()

    def on_mount(self) -> None:
        """Start a background refresh loop."""
        self.refresh_state()

    def compose(self) -> ComposeResult:
        yield Header()
        yield self._input
        yield Footer()

    @work
    async def refresh_state(self) -> None:
        """This stands for a more complicated refresh state loop."""
        while True:
            self.set_focus(self._input)
            await asyncio.sleep(0.1)


class ReproApp(App[None]):

    AUTO_FOCUS = None  # Important for reproduction!

    def __init__(self) -> None:
        super().__init__()

    async def on_mount(self) -> None:
        self.initialize_welcome_screen()

    @work
    async def initialize_welcome_screen(self) -> None:
        await self.push_screen(ReproScreen())


async def test_repro_app() -> None:
    app = ReproApp()
    async with app.run_test():
        pass


if __name__ == "__main__":
    asyncio.run(ReproApp().run_async())
Output from `textual diagnose` ``` # Textual Diagnostics

Versions

Name Value
Textual 0.70.0
Rich 13.7.1

Python

Name Value
Version 3.12.1
Implementation CPython
Compiler GCC 9.4.0
Executable /home/saf/code/es/editshare-system-upgrade/.venv/bin/python3

Operating System

Name Value
System Linux
Release 5.4.0-186-generic
Version #206-Ubuntu SMP Fri Apr 26 12:31:10 UTC 2024

Terminal

Name Value
Terminal Application vscode (1.90.2)
TERM xterm
COLORTERM truecolor
FORCE_COLOR Not set
NO_COLOR Not set

Rich Console options

Name Value
size width=169, height=18
legacy_windows False
min_width 1
max_width 169
is_terminal False
encoding utf-8
max_height 18
justify None
overflow None
no_wrap False
highlight None
markup None
height None
</details>

Feel free to add screenshots and / or videos. These can be very helpful!
Copy link

We found the following entry in the FAQ which you may find helpful:

Feel free to close this issue if you found an answer in the FAQ. Otherwise, please give us a little time to review.

This is an automated reply, generated by FAQtory

@willmcgugan
Copy link
Collaborator

Have you managed to reproduce this outside of pytest?

@slawek-es
Copy link
Author

slawek-es commented Jun 26, 2024

Have you managed to reproduce this outside of pytest?

Well, if you change the reproduction module to say:

if __name__ == "__main__":
    asyncio.run(test_repro_app())

then the issue technically happens outside of pytest, but it still uses App.run_test. Do you think this is a deciding factor here?

Apart from this, I have been unable to reproduce this issue by running the application normally.

slawek-es added a commit to slawek-es/textual that referenced this issue Jun 27, 2024
There is a deadlock issue on `App._dom_lock`, where `_close_all` holds
the lock and waits for the task associated with a message pump, but
that task may be executing a "called-next" `_prune_nodes` call driven
by an `AwaitRemove` which was triggered, for example, by the
`Footer.recompose` call from handling the `bindings_updated`
signal. This deadlock leads to a `TimeoutError` on app close.

We fix this by omitting the call to `_prune_nodes` in the
`AwaitRemove` task if the whole application is already closing; it
does not make sense to remove individual nodes in this case anyway.

Fixes Textualize#4677
@willmcgugan
Copy link
Collaborator

Fixed in main.

Copy link

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants