Regression in Textual 0.48 when executing app in a subprocess

[pablogsal]

In the memray project we have several tests that execute textual in a subprocess and drives them by passing input. For example:

https://github.com/bloomberg/memray/blob/cc5463a4c4fca83b53a48331501e1753a78e39ca/tests/integration/test_main.py#L853-L874

Since yesterday's release of Textual 0.48 all of these test hang and our test suite is frozen. It looks like the app ignores the input that we are passing via input="q". I can confirm that all these test work with textual<0.48.

Unfortunately this is blocking memray's CI right now

[github-actions]

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

[pablogsal]

Bisected to this commit:

374478a0b12640df4753c041770e824a2c4259f0 is the first bad commit
commit 374478a0b12640df4753c041770e824a2c4259f0
Author: Dave Pearson <davep@davep.org>
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(-)

[pablogsal]

Looks like this call:

374478a#diff-13d6e180136893233a19e971fc665690e6afb9ed7b807311884ab961f1db25f2R157-R159

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

[willmcgugan]

Thanks. Will look at that soon.

[pablogsal]

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

[pablogsal]

I think this logic is flawed:

textual/src/textual/drivers/linux_driver.py

Lines 173 to 176 in 8ae0b27

	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).
	return

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.

[davep]

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).

[pablogsal]

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.

[davep]

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. :-)

[davep]

@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.

[pablogsal]

@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

[davep]

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

[pablogsal]

Thanks a lot for the help! ❤️

[davep]

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

[github-actions]

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

[willmcgugan]

Fixed in 0.48.2, just released