Invoke==0.12 waits for keypress and gets Broken Pipe when invoking another invoke command through run()

[tuukkamustonen]

On linux, python 2.7 and invoke==0.12:

from invoke.collection import Collection
from invoke.tasks import ctask

@ctask
def sub(ctx):
    ctx.run('inv hello')

@ctask
def hello(ctx):
    ctx.run('echo hello')

ns = Collection(sub, hello)

Works:

$ inv hello
hello

Fails:

$ inv sub
hello
<stops to wait for a keypress here...>
Traceback (most recent call last):
  File "/home/musttu/.virtualenvs/pyinv/bin/inv", line 11, in <module>
    sys.exit(program.run())
  File "/home/user/.virtualenvs/pyinv/local/lib/python2.7/site-packages/invoke/program.py", line 270, in run
    self.execute()
  File "/home/musttu/.virtualenvs/pyinv/local/lib/python2.7/site-packages/invoke/program.py", line 379, in execute
    executor.execute(*self.tasks)
  File "/home/musttu/.virtualenvs/pyinv/local/lib/python2.7/site-packages/invoke/executor.py", line 114, in execute
    result = call.task(*args, **call.kwargs)
  File "/home/musttu/.virtualenvs/pyinv/local/lib/python2.7/site-packages/invoke/tasks.py", line 113, in __call__
    result = self.body(*args, **kwargs)
  File "/home/musttu/Temp/tasks.py", line 12, in hello
    ctx.run('echo hello')
  File "/home/musttu/.virtualenvs/pyinv/local/lib/python2.7/site-packages/invoke/context.py", line 53, in run
    return runner_class(context=self).run(command, **kwargs)
  File "/home/musttu/.virtualenvs/pyinv/local/lib/python2.7/site-packages/invoke/runners.py", line 232, in run
    raise ThreadException(exceptions)
invoke.exceptions.ThreadException: 
Saw 1 exceptions within threads (OSError):


Thread args: {'kwargs': {'input_': <open file '<stdin>', mode 'r' at 0x7fee83ac00c0>},
 'target': <bound method Local.handle_stdin of <invoke.runners.Local object at 0x7fee81e7d390>>}

Traceback (most recent call last):

  File "/home/musttu/.virtualenvs/pyinv/local/lib/python2.7/site-packages/invoke/runners.py", line 804, in run
    super(_IOThread, self).run()

  File "/usr/lib/python2.7/threading.py", line 763, in run
    self.__target(*self.__args, **self.__kwargs)

  File "/home/musttu/.virtualenvs/pyinv/local/lib/python2.7/site-packages/invoke/runners.py", line 433, in handle_stdin
    self.write_stdin(self.encode(data))

  File "/home/musttu/.virtualenvs/pyinv/local/lib/python2.7/site-packages/invoke/runners.py", line 653, in write_stdin
    return os.write(fd, data)

OSError: [Errno 32] Broken pipe

In 0.11.1 it works just ok.

The use case: While this example doesn't make much sense, in the real world, I'm invoking another invoke commands in sub-directories from a "master" tasks.py.

[presidento]

It is the same in Win64 with Python 3.4, except I got the exception using inv hello, and inv sub cannot be killed with Ctrl+C.

C:\Users\user\temp\pyinvoke test>inv hello
hello
Traceback (most recent call last):
  File "c:\programs\python34\lib\runpy.py", line 170, in _run_module_as_main
    "__main__", mod_spec)
  File "c:\programs\python34\lib\runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "C:\Programs\Python34\Scripts\inv.exe\__main__.py", line 9, in <module>
  File "c:\programs\python34\lib\site-packages\invoke\program.py", line 270, in run
    self.execute()
  File "c:\programs\python34\lib\site-packages\invoke\program.py", line 379, in execute
    executor.execute(*self.tasks)
  File "c:\programs\python34\lib\site-packages\invoke\executor.py", line 114, in execute
    result = call.task(*args, **call.kwargs)
  File "c:\programs\python34\lib\site-packages\invoke\tasks.py", line 113, in __call__
    result = self.body(*args, **kwargs)
  File "C:\Users\user\temp\pyinvoke test\tasks.py", line 10, in hello
    ctx.run('echo hello', shell=True)
  File "c:\programs\python34\lib\site-packages\invoke\context.py", line 53, in run
    return runner_class(context=self).run(command, **kwargs)
  File "c:\programs\python34\lib\site-packages\invoke\runners.py", line 232, in run
    raise ThreadException(exceptions)
invoke.exceptions.ThreadException:
Saw 1 exceptions within threads (OSError):


Thread args: {'kwargs': {'input_': <_io.TextIOWrapper name='<stdin>' mode='r' encoding='cp852'>},
 'target': <bound method Local.handle_stdin of <invoke.runners.Local object at 0x02EF6230>>}

Traceback (most recent call last):

  File "c:\programs\python34\lib\site-packages\invoke\runners.py", line 804, in run
    super(_IOThread, self).run()

  File "c:\programs\python34\lib\threading.py", line 868, in run
    self._target(*self._args, **self._kwargs)

  File "c:\programs\python34\lib\site-packages\invoke\runners.py", line 411, in handle_stdin
    reads, _, _ = select.select([input_], [], [], 0.0)

OSError: [WinError 10093] Either the application has not called WSAStartup, or WSAStartup failed

[tuukkamustonen]

I can kill inv sub with Ctrl+C but if I do that, I get into situation reported in #303.

[bitprophet]

Thanks for the reports! Confirm I can reproduce locally.

On poking, noted that the ThreadError/OSError is in the "inner" invoke process (hello, not sub).

Also noted that the behavior is improved if sub uses run(pty=True); or, unsurprisingly, if both do (can't just put it on hello only though - the inner Invoke will notice the outer isn't a pty and fallback to non-pty execution, as expected).

Handing in_stream=StringIO("") to sub only changes things insofar as sending the real-stdin newline now makes no difference and one is forced to Ctrl-C.

So far I'm assuming there's a race condition or other stupidity going on wrt the input mirror loop (which loops until it receives the signal from another thread that the subprocess has exited). Why this is only triggered by nesting is unclear offhand. Will dig.

[bitprophet]

Also linking this explicitly to #289 since it came out of that feature.

[bitprophet]

Right - it's because Invoke doesn't appear to be a terminal when not in pty mode (because...it's not), and Invoke checks its own stdin for terminal-ness when deciding how to 'wait' on stdin during the stdin read loop:

invoke/invoke/runners.py

Lines 413 to 447 in 7fe2750

	with character_buffered(input_):
	while True:
	byte = None
	if ready_for_reading(input_):
	# Read 1 byte at a time for interactivity's sake.
	byte = read_byte(input_)
	# Short-circuit if not a real stream and appeared to hit
	# end-of-file.
	if not isatty(input_) and not byte:
	break
	# Mirror what we just read to process' stdin.
	# We perform an encode so Python 3 gets bytes (streams +
	# str's in Python 3 == no bueno) but skip the decode step,
	# since there's presumably no need (nobody's interacting
	# with this data programmatically).
	# TODO: consider baking the encode call into write_stdin
	# (needs indirection as actual impl of write_stdin differs
	# between subclasses...)
	# TODO: will this break with multibyte input character
	# encoding?
	self.write_stdin(self.encode(byte))
	# Also echo it back to local stdout (or whatever
	# out_stream is set to) when no pty has been allocated
	# (ptys will perform their own echoing).
	if not self.using_pty:
	output.write(byte) # TODO: encode?
	output.flush()
	# Dual all-done signals: program being executed is done
	# running, *and* we don't seem to be reading anything out of
	# stdin. (If we only test the former, we may encounter race
	# conditions re: unread stdin.)
	if self.program_finished.is_set() and not byte:
	break
	# Take a nap so we're not chewing CPU.
	time.sleep(self.input_sleep)

When a real pty is present (again, this is the case if one uses pty=True in the outer invocation) the "is stdin ready for reading?" function at the top of said loop will never block: it uses select (or Windows equiv), returning False immediately, forever; the loop is thus "hot" (except it doesn't actually chew CPU due to a sleep call 😸).

In the case under test, where there is no intended stdin data, this means everything lines up well: that loop runs often, eventually the "is the subprocess done?" test triggers, the loop exits, everything closes up shop, we're done.

In the error case, when the input stream doesn't appear to be a terminal (via our custom isatty function) ready_for_reading() always returns True on the presumption that the stream is an in-process Python object or similar finite-length file-like object that can be read(1) from.

So we end up sitting in read_byte, where input_ is an actually-kinda-real sys.stdin, until a byte appears in the stream (this is where 'hitting a key' enters the picture). But at this point in time, the echo subprocess has already exited, thus the OSError on broken pipe when the inner Invoke tries mirroring this read byte into it.

---

tl;dr: we need at least one of the following:

• Invoke needs to pass isatty tests from the perspective of its subprocesses, at least when it's mirroring a real TTY.
  • This is probably difficult or impossible? but I need to think about it because it'd be "nice" for things to be that transparent - I've always seen the ideal here for Invoke to be indistinguishable from lower-level C-based programs that do similar stuff.
• Or, the stdin mirroring functionality needs to modify its assumptions about the stdin stream object involved, such that it is able to detect this sort of "is still a real-enough tty that may never have actual data coming through it" situation.
  • This presumes that using select/getch against an outer Invoke process (one not using pty=True) even works, which depends in part on what subprocess is doing exactly...
    • If they don't work in this case, we may be just screwed, since neither blocking nor nonblocking reads would then be possible, unless I'm blind to something.
  • If they do, we need to figure out the gap that exists here re: TTY detection, which currently is "does it have an .isatty? and if not does it have a .fileno?".
    • Alternately, we need to alter ready_for_reading in similar fashion, tho that brings us back to "what other ways are there to determine if there's data in this pipe?", and we need something that can accurately answer "No" without blocking.

To be continued...

[bitprophet]

Examining the state of my local setup, I found that while sys.stdin.isatty is False, sys.stdin.fileno is 0. Right now, our isatty test prefers .isatty and completely disregards .fileno if .isatty is callable. However, the case under consideration is exactly the kind of valid setup the code disregards.

It should be the same as some other non-Invoke program piped as stdin to Invoke, which is another case I think we have bug reports for (from earlier) as well. Sure enough, echo 'lolnope' | inv hello yields the exact same set of errors.

So our fixation on "tty-ness" for purposes of how to read from stdin is misguided and needs fixing.

Offhand I am thinking making it "either-or" should suffice (says it's a tty? cool! says it's not a tty, but has a fileno? also cool!) but I need to revisit the reasons why we made it the way it is now (I recall other edge/corner cases being involved).

May also involve splitting the method up a bit - some cases we really do care if it's a TTY, IIRC.

EDIT: yea, here's all calls to utils.isatty at present:

• Within char_buffered (since that involves using termios etc) - this is a real, useful/full use of the func, presumably.
• Within handle_stdin, similar to the other uses at hand in this ticket: it's trying to test whether the stream is a finite file-like object or not so it knows whether an empty byte means EOF.
  • Though now that I think on it, an empty read byte should always imply EOF; a real stdin stream would never yield one...
• Local pty fallback cares, so it doesn't try to allocate a pty when there isn't one on the stdin side to hook it up to.
  • Think this falls under the same category as char_buffered, but not sure.

---

What I need to do:

• Ensure we have integration suite tests (possibly extending to "shit run in .travis.yml itself" to avoid this kind of invoke-within-invoke problem?) testing scenarios like this one & the 'normal pipe to stdin' version
• See how bad our existing tests blow up if I just change utils.isatty to be more expansive in scope
• Assuming it does, examine recent history re: that function (I know pfmoore and I kicked it around a bit re: Windows behavior, for example) to determine best way to split it up so we can ask these semi-separate questions at appropriate times:
  • Can I use select/getch on you (for nonblocking reads)?
  • Can I call termios on you (to set character-buffered stdin)?
  • Can I use the pty & fcntl modules on you (and also termios actually) (to spawn a pty for running the subprocess within)?

[bitprophet]

Minor update, I read isatty slightly too fast, the fallback test isn't "does it have a real fileno?" but "does it have a real fileno and is that fileno a tty according to os.isatty?". Which is of course, still False in this particular scenario (both cases).

Also, FTR, the reason it's always false here, but fileno itself is "okay", is because we're just inheriting file handles all the way down; so sys.stdin.fileno() is still 0, like "normal" ("normal" being "a Python process with nothing else in the way of standard input"). But it's not a TTY because either the pipe, or Invoke, is in the way, depending. (N.B. there's now one test for each situation in the integration suite.)

This is definitely hedging me further towards "we need to separate the questions of TTY-ness and select-ability", which I think will solve this pretty well.

[bitprophet]

So yea, re: previous comment, updating isatty to be either-or doesn't help any, as the os.isatty test around the stream's fileno() still fails. Definitely do need separate functions. Working on that now.

[bitprophet]

Dug a little deeper into our use of isatty and friends:

• The select/getch from ready_for_reading is both the heart of the issue here & the most straightforward, it should boil down to "can I get a numeric fileno from the stream?", so e.g. an inherited sys.stdin fd of 0 ought to be happy here.
• character_buffered uses two calls:
  • termios.tcgetattr(stream) - all termios calls also want a working fileno (either an object w/ a .fileno() returning an int, or just the int) so again, inherited sys.stdin good, StringIO instance bad.
  • tty.setcbreak(stream) - says it wants an 'fd' and also mentions termios, so presumably same deal here - only cares about fd.
    • However! This is one that might be slightly pissy with something that fails stream.isatty(), will have to test.
• The stuff in Local is all predicated on self.using_pty, whose fallback functionality is what asks isatty(sys.stdin). The calls made if self.using_pty is True are:
  • pty_size() which asks sys.stdout (NOT sys.stdin) if it's a tty, using sys.stdout.isatty
    • Shouldn't this be using out_stream instead? Hrm.
    • EDIT: well, but the windows branch of the code doesn't touch sys.stdout at all - there'd be nothing to parameterize on. Also seems unlikely that someone would pass in a real terminal stream for out_stream that somehow isn't sys.__stdout__. Punting =/
  • fcntl.ioctl(sys.stdout.fileno()) - presumably still good with "fd OK, even if isatty false"
  • pty.fork - this presumably cares about actual tty-ness, but need to test

Need to doublecheck to make sure I'm not missing anything, but so far, it does feel like the majority of these actually just care about fileno not isatty - need to check git history too because I'm pretty sure isatty() used to care more about fileno so if so, I wonder why things changed - suspect Windows related?

[bitprophet]

Doublechecked, everything should be listed above now.

Re: git history, we did previously rely mostly on fileno (though this was prior to implementing the stdin mirroring so was mostly used for the Local pty stuff I think), then in 57f7f42 it got changed to prefer isatty (and then later in 61feb22 we expanded to the current setup of "isatty, then fileno").

I still think that this back & forth is due to the two different questions involved re: Local pty execution jazz vs the concerns about reading from stdin, so I'm gonna steam ahead with that and hope anything it breaks bubbles up quickly.

[bitprophet]

Poked a bit re: above:

• When Python is running inside an Invoke run(pty=False), sys.stdout behaves same as sys.stdin, insofar as .fileno() works happily (& returns 1) but .isatty() is False.
  • When run(pty=True), isatty is True, as expected
• Replacing the outer Invoke's sys.stdin and sys.stdout doesn't impact the inner Python process at all, re: obtaining filenos.
• Whether isatty is True or False in the inner Python process depends solely on whether the outer Invoke makes use of pty.fork or not - expectedly.
• Forcing pty=True, fallback=False with StringIOs as sys.stdin/out causes things to blow up (again, expected) when fcntl.ioctl runs (in Local.start) and finds out StringIO lacks fileno. This isn't strictly related but eh. Poked it anyways.
• pty.fork() actually doesn't seem to care about isatty-ness either; it's happy to execute even if sys.stdin or sys.stdout are isatty==False. This is more fuel for "caring about fileno is the only thing that really matters" I think.

[bitprophet]

Grump, went ahead and tried flipping everything back to fileno-only testing, but I obviously didn't test enough. Even though we have useful fileno on sys.stdin in a nested-Invoke situation, character_buffered's call to termios.tcgetattr gets upset and throws Inappropriate ioctl for device.

So we probably do still need a mixture of isatty and fileno checks...testing. EDIT: yup, if we update character_buffered to test isatty but leave everything else testing fileno, the test case here (re: inv sub) now works for me.

[bitprophet]

Test suites now finding some more quirks I hadn't run into yet, firstly: an integration test that incidentally calls invoke-from-invoke (testing --help output) yields an IOError of Inappropriate ioctl for device in pty_size's use of fcntl.ioctl.

• In this case, sys.stdout has fileno 1 but isatty says False.
• Implication is thus that my guess above about "fileno is sufficient" doesn't play true here either.
• Could switch this spot back to checking isatty, though I do wonder if there are situations where "has fileno, is not tty" would work.
• Perhaps the right thing for this particular method is to employ EAFP - in fact, we do already wrap the breaking lines in try/except, we just only check for AttributeError, and going by its comments that's actually a holdover from earlier lack of rigorous fileno/isatty checking. So we may just want to add/change the except to look for IOError...