Regression in Textual 0.48 when executing app in a subprocess about textual HOT 16 CLOSED

Fixed in 0.48.2, just released

from textual.

Looks like this call:

374478a#diff-13d6e180136893233a19e971fc665690e6afb9ed7b807311884ab961f1db25f2R157-R159

is making the process hang. Seems that termios.tcgetattr is enough to make it hang

from textual.

@pablogsal I don't have anything to hand similar to what you'e doing (I'll try and make an MRE, unless you have one), but if you're in a position to, just as a quick test, presumably something akin to this:

diff --git a/src/textual/drivers/linux_driver.py b/src/textual/drivers/linux_driver.py
index 7f33082d..9cadc5c0 100644
--- a/src/textual/drivers/linux_driver.py
+++ b/src/textual/drivers/linux_driver.py
@@ -167,9 +167,10 @@ class LinuxDriver(Driver):
             # output; so rather than get into the business of spinning up
             # application mode again and then finding out, we perform a
             # no-consequence change and detect the problem right away.
-            termios.tcsetattr(
-                self.fileno, termios.TCSANOW, termios.tcgetattr(self.fileno)
-            )
+            if os.isatty(self.fileno):
+                termios.tcsetattr(
+                    self.fileno, termios.TCSANOW, termios.tcgetattr(self.fileno)
+                )
         except termios.error:
             # There was an error doing the tcsetattr; there is no sense in
             # carrying on because we'll be doing a SIGSTOP (see above).

If you're in a position to give that or similar a spin I'd be curious to hear how it goes.

I can confirm that this also fixes the problem

from textual.

Thanks a lot for the help! ❤️

from textual.

Thank you for your issue. Give us a little time to review it.

PS. You might want to check the FAQ if you haven't done so already.

This is an automated reply, generated by FAQtory

from textual.

Bisected to this commit:

374478a0b12640df4753c041770e824a2c4259f0 is the first bad commit
commit 374478a0b12640df4753c041770e824a2c4259f0
Author: Dave Pearson <[email protected]>
Date:   Mon Jan 29 13:55:47 2024 +0000

    Move the main work on suspending with Ctrl+Z into the Linux driver

 src/textual/app.py                  |  6 +----
 src/textual/drivers/linux_driver.py | 46 +++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 5 deletions(-)

from textual.

Thanks. Will look at that soon.

from textual.

I did some extra debugging, it's not hanging in termios.tcgetattr. That call is raising termios.error: (25, 'Inappropriate ioctl for device') and then the process just hangs in the even loop not processing the input:

  File "/usr/lib/python3.10/selectors.py", line 469, in select
    fd_event_list = self._selector.poll(timeout, max_ev)
  File "/usr/lib/python3.10/asyncio/base_events.py", line 1871, in _run_once
    event_list = self._selector.select(timeout)
  File "/usr/lib/python3.10/asyncio/base_events.py", line 603, in run_forever
    self._run_once()
  File "/usr/lib/python3.10/asyncio/base_events.py", line 636, in run_until_complete
    self.run_forever()
  File "/usr/lib/python3.10/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/tmp/venv/lib/python3.10/site-packages/textual/app.py", line 1496, in run
    asyncio.run(run_app())
  File "/src/src/memray/reporters/tree.py", line 513, in render
    self.get_app().run()
  File "/src/src/memray/commands/tree.py", line 80, in run
    reporter.render()
  File "/src/src/memray/commands/__init__.py", line 138, in main
    arg_values.entrypoint(arg_values, parser)
  File "/src/src/memray/__main__.py", line 6, in <module>
    sys.exit(main())
  <built-in method exec of module object at remote 0xffff92020950>
  File "/usr/lib/python3.10/runpy.py", line 86, in _run_code

from textual.

I think this logic is flawed:

Here, it's clear that termios.tcgetattr will raise if sys.stdin is a PIPE so returning from the function will not receive any SIGSTOP and will leave the app hanging. I can confirm that substituting the return for a pass fixes the issue.

from textual.

I can confirm that substituting the return for a pass fixes the issue.

That, however, then breaks the handling of being suspended and attempts to bg the task, etc.

Excellent spot though, thanks for narrowing this down.

For a bit of background, so I can create a wee test to juggle the competing needs: you're running a Textual application in an automated way, in a subprocess? Is it the case that the display itself isn't needed? (just randomly wondering if this sort of thing should be going via the headless driver, for example).

from textual.

That, however, then breaks the handling of being suspended and attempts to bg the task, etc.

Right but this problem highlights that that logic is assuming that the only situation for termios.error is the one described in the paragraph and that is not correct. Maybe we can consider doing that conditional to if stdin is a tty or not.

you're running a Textual application in an automated way, in a subprocess?

Yes, for testing purposes. See the link in the first comment as reference

Is it the case that the display itself isn't needed? (just randomly wondering if this sort of thing should be going via the headless driver, for example).

We parse the output from the subprocess, but we do not need to "see" it for this tests.

from textual.

Right but this problem highlights that that logic is assuming that the only situation for termios.error is the one described in the paragraph and that is not correct. Maybe we can consider doing that conditional to if stdin is a tty or not.

Yup, got that, I was just making a note for the issue in general that the change you mentioned has a (intended) consequence; just making notes.

Amusingly I was just looking at handling it with some use of isatty. :-)

from textual.

@pablogsal I don't have anything to hand similar to what you'e doing (I'll try and make an MRE, unless you have one), but if you're in a position to, just as a quick test, presumably something akin to this:

diff --git a/src/textual/drivers/linux_driver.py b/src/textual/drivers/linux_driver.py
index 7f33082d..9cadc5c0 100644
--- a/src/textual/drivers/linux_driver.py
+++ b/src/textual/drivers/linux_driver.py
@@ -167,9 +167,10 @@ class LinuxDriver(Driver):
             # output; so rather than get into the business of spinning up
             # application mode again and then finding out, we perform a
             # no-consequence change and detect the problem right away.
-            termios.tcsetattr(
-                self.fileno, termios.TCSANOW, termios.tcgetattr(self.fileno)
-            )
+            if os.isatty(self.fileno):
+                termios.tcsetattr(
+                    self.fileno, termios.TCSANOW, termios.tcgetattr(self.fileno)
+                )
         except termios.error:
             # There was an error doing the tcsetattr; there is no sense in
             # carrying on because we'll be doing a SIGSTOP (see above).

If you're in a position to give that or similar a spin I'd be curious to hear how it goes.

from textual.

Awesome, thanks, and that also works as intended when handling suspending, bging, etc.

from textual.

Back at you; helpfully detailed and easily tested report. Really appreciate it.

from textual.

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

from textual.