Blocking when running commands looking for stdin close

[plockc]

Example:

@task
def cat(c):
    c.run("cat")

echo Hi | timeout 3 invoke cat || echo failed

This prints Hi but never exits because cat never receives a close of stdin and is waiting for more, and invoke is waiting on the subprocess.

In a different issue, I notice that the console has trouble after running the above command

Caused by two bugs:

First is that ExceptionHandlingThread.is_dead return expression is wrong, it should be "or" instead of "and":
https://github.com/pyinvoke/invoke/blob/master/invoke/util.py#L274

Second, is that the stdin for the process is not closed after receiving EOF:
https://github.com/pyinvoke/invoke/blob/master/invoke/runners.py#L659

Python 3.6.5
Ubuntu 18.04
Invoke 1.0.0.0

[bitprophet]

Can confirm the lack of exiting easily enough.

Not getting terminal weirdness however, even under bash, which is where I am able to reproduce the potentially-related fabric/fabric#1812. Still trying to determine if the two issues stem from the same bugs.

[bitprophet]

Still ongoing but that Fabric ticket seems to be caused by executing with character_buffered(...) multiple times concurrently (such that all but the first invocation erroneously snapshots, then restores, an already-character-buffered terminal state) which is what causes the weird local terminal behavior (specifically, the echo flag is set to off, and then never properly restored).

Given the code is a contextmanager, I don't see why it would ever hit that particular issue in single-threaded Invoke-only code; no matter what happens inside the block (early exit, error, process termination short of SIGKILL) the post-yield, end-of-block terminal restoration code (which is in a finally clause) should always run.

In addition, when (our) stdin is hooked up to a pipe, as it is here, that contextmanager should become a no-op. The detection of whether to do that has historically been a bit fragile, so maybe this exposes another hole in it. (Though that still shouldn't cause a problem here unless the file descriptor in question is switching targets underneath us - e.g. it's pointing at the echo output pipe initially, which might yield weird tcgetattr results [tho I'd expect an explosion instead], then if we restore those onto the real stdin after for some reason...?)

Either way, I can't reproduce that angle, but I can reproduce the hanging/never-exiting problem so I'll focus on that first and we can make a new issue for terminal state corruption if necessary.

[plockc]

hi @bitprophet, the terminal weirdness only happened once, I can't reproduce, I probably should not have mentioned in this ticket.

I have locally ran 1.1.1 and still can reproduce the blocking issue, while taking my 1.0.0 patch and rebasing on top of 1.1.1 works for the "cat" test.

I added test cases and a discussion about why I think the test cases were wrong too, mind looking at my comments and the test case updated assertions?
#553

[bitprophet]

Think I'm hitting this internally now and traced it to something similar-but-different - a lack of submitting EOF down the pipe when non-mirrored in_stream hits its end.

I suspect that the explicit file close in OP's commit (https://github.com/plockc/invoke/commit/4d6ab1143ae998bc1bc97862ac40974716511d5e) will do the same thing and will test that momentarily.

FWIW my own fix of Runner.send_interrupt() results in a later thread deadlock / logic hang (we hit the or logic in Runner.wait, where dead_threads is True but proc_finished is still False) so if that still pops up after switching to the file close approach, more work will be needed.

EDIT: actually when I read that commit's message I see the same problem mentioned there. Duh!

[bitprophet]

When looking at extending that commit to Fabric (which is where I am troubleshooting this for now) I notice we have a mostly-noop Runner.stop method. Thinking maybe we want to fold _close_proc_stdin into that method (or at least call the latter from the former, then call the former where the patch currently calls the latter...). Still figuring out if this even fixes the whole problem for me though.

[bitprophet]

Does appear to fix my issue, however there's some race condition where maybe 10% of the time, the remote end (in this case, a simple wc -l) exits -1 instead of 0. Gotta remember exactly what that means, though my offhand guess is it means we're closing down before the remote end is and it's getting a broken pipe.

EDIT: when switching to a Linux workstation, the incidence of the problem goes way up (or is 100% of the time perhaps). Guess that'll be useful...

[bitprophet]

Interestingly, the bad exit code happens even when the remote program doesn't touch its stdin. I also realize...there's no stdout coming back down the pipe. So something's very clearly wrong here. Removing in_stream from the equation makes everything normal again, even with this patch involved.

The issue with lack of stdout may be a separate, related logic error triggered by in_stream though. EDIT: perhaps not. when this problem appears the remote program doesn't even seem to run at all...eg if I put in a sleep, it doesn't hold things up, if I exit 7, the local end always thinks the returncode is -1. So something's going seriously sideways here.