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

Unblock subprocesses waiting for stdin close #553

Open
wants to merge 4 commits into
base: master
from

Conversation

@plockc
Copy link

commented Jul 8, 2018

Partial fix for #552

New method added _close_proc_stdin to allow the parent Runner to ask
subclass to close the stream after it finds the end of the stream. If the subclass
has not implemented _close_proc_stdin yet, then it is a no-op (and the bug
remains)

Fix ExceptionHandlingThread.is_dead which before would report dead when
thread was not alive and there was no exception info

This does not fix when pty=True, more significant changes are required for that case

Two test cases were adjusted that were wrongly asserting that the thread
should be "is_dead" after thread.join()

@plockc

This comment has been minimized.

Copy link
Author

commented Jul 9, 2018

The appveyor seems to fail on many (all?) jobs with failing to find:
C:\Python27\Scripts\spec.exe

@@ -659,6 +659,8 @@ def handle_stdin(self, input_, output, echo):
elif data is not None:
# When reading from file-like objects that aren't "real"
# terminal streams, an empty byte signals EOF.
if not self.using_pty:
self._close_proc_stdin()

This comment has been minimized.

Copy link
@plockc

plockc Jul 9, 2018

Author

can check for the existence of attribute on self to allow subclasses to defer the fix

@bitprophet

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

I'm a bit leery about the is_dead change since you're changing the test to just match the new behavior. Been a while since I looked at this part of the code, but I'm guessing the original author explicitly intended the is_dead flag to exhibit the properties it did in the test. Inverting the logic seems like a bad idea?

Can you provide more details about how the current test and implementation are both wrong? Thanks!

@plockc

This comment has been minimized.

Copy link
Author

commented Jul 11, 2018

Hi @bitprophet I have to run but had time to answer some of your concerns, I can cover the two usage locations of is_dead usage later if you need:

Original code:

return (not self.is_alive()) and self.exc_info is not None

New code:

alive = self.is_alive() and (self.exc_info is None)
        return not alive

For new code, if a is "is_alive" and b is "self.exc_info is None", we can substitute and then rearrange for the is_dead calculation:

!alive
!(a and b) 
!a or !b  # demorgan's
!(is_alive) or !(self.exc_info is None)
not self.is_alive or self.exc_info is not None

Which is exactly the same as the original except using or instead of and


Consider the case where the thread completes normally, the thread's is_alive will be False, in the original code:

(not self.is_alive()) and self.exc_info is not None
(not False) and self.exc_info is not None
True and self.exc_info is not None
self.exc_info is not None

Which means: is_dead == has exc_info . . . but if we exit normally, there would be no exception captured, and so is_dead reports false, keeping the input thread alive


The test case was implemented incorrectly to make it pass, here is one of them:
https://github.com/plockc/invoke/blob/master/tests/concurrency.py#L39-L42

 t = EHThread(target=self.worker, args=[Queue()])
 t.start()
 t.join()
assert not t.is_dead

The last line is asserting that the thread is alive, right after a thread join, thread.join() docs: "Wait until the thread terminates"

@plockc

This comment has been minimized.

Copy link
Author

commented Jul 11, 2018

i updated the tests for additional validation of underlying conditions, and the call to the new close stdin is now checked first to allow subclasses in other repos (fabric?) to update on it's own schedule

I think we'll want to squash the commmits, kept separate for review purposes

@plockc

This comment has been minimized.

Copy link
Author

commented Jul 11, 2018

I didn't call it out here, but there is a minimal test scenario in #552

Chris Plock added some commits Jul 8, 2018

Chris Plock
Unblock subprocesses waiting for stdin close
Partial fix for #552

BREAKING CHANGE FOR RUNNER IMPLEMENTATIONS:
New method added close_proc_stdin to allow the parent Runner to ask
subclass to close the stream after it finds the end of the stream.

Fix ExceptionHandlingThread.is_dead which before would report dead when
thread was not alive and there was no exception info

This does not fix when pty=True, more significant changes are required for that case

Two test cases were adjusted that were wrongly asserting that the thread
should be "is_dead" after thread.join()
Chris Plock
@plockc

This comment has been minimized.

Copy link
Author

commented Sep 3, 2018

To make things more interesting, occassionally (maybe 1/4 of the time), I get an UnexpectedExit error in program.py:run (running the same task) when I have in_stream=StringIO(meta_data):

Encountered a bad command exit code!

Command: 'cd /var/lib/libvirt/images/base && sudo tee meta-data > /dev/null'

Exit code: None

Stdout: already printed

Stderr: already printed

I don't know yet if it's something I introduced, or pointing to something deeper and darker.

The Result is false, printing out the Result shows:

Command was not fully executed due to watcher error.
(no stdout)
(no stderr)

Seems like there was some handling for watchererror but that's not being activated by what result is showing, and not sure yet why/where there is a watcher error.

I think the process hasn't finished terminating / set the returncode, if i return poll() for returncode, I can't repro the issue . . not sure why we'd get past all the thread joins though while process is still alive though.

@74th 74th referenced this pull request Sep 15, 2018
@seansfkelley

This comment has been minimized.

Copy link

commented Nov 10, 2018

This issue is making it impossible to write a task that feeds a string to the stdin when shelling out. I'm doing something along these lines:

@task
def t(c):
  input_string = ...
  c.run("program --that waits/until/stdin/ends", in_stream=StringIO(input_string))

The process just hangs forever, because the program won't do anything until stdin is exhausted.

A really simple repro:

from invoke import task
from io import StringIO

@task
def t(c):
    c.run("cat", in_stream=StringIO("foo"))

If you run this task, it should print 'foo' and then hang forever.

I tried all manner of combinations of closeing the stream before/after the call or writing control characters to the stream, but no dice.

@bitprophet what needs to be done to get this PR merged?

See also #575 (comment).

@shvechikov

This comment has been minimized.

Copy link

commented Nov 13, 2018

I'm having the same issue as @seansfkelley — just trying to send dynamic input to a process using in_stream=StringIO(...) parameter and it hangs.
@bitprophet Do we do something wrong?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.