Skip to content

Give the process a chance to terminate correctly #661

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

Closed
wants to merge 1 commit into from

Conversation

flazzarini
Copy link

Should fix the issue described in #660

@florisla
Copy link
Contributor

The CI build fails because Popen.wait() does not have a timeout argument in Python 2.7.

See https://docs.python.org/2/library/subprocess.html#subprocess.Popen.wait

Would it be dangerous to simply call wait() in case of python <3 ?

@flazzarini
Copy link
Author

Yes I didn't think of python 2 support at all, but as python 2 is end of life on 01.01.2020 will you also drop support for it in the invoke library? If this is the case this fix could applied on a python3 only branch.

rsora pushed a commit to arduino/arduino-cli that referenced this pull request Sep 23, 2019
In order to overcome issue reported here
#416
we used successfully this patch
pyinvoke/invoke#661
in the requirements.txt so we re-enable the
test pipeline step for e2e tests on win
rsora pushed a commit to arduino/arduino-cli that referenced this pull request Sep 24, 2019
In order to overcome issue reported here
#416
we used successfully this patch
pyinvoke/invoke#661
in the requirements.txt so we re-enable the
test pipeline step for e2e tests on win
rsora pushed a commit to arduino/arduino-cli that referenced this pull request Sep 24, 2019
* Use patched invoke in e2e requirements

In order to overcome issue reported here
#416
we used successfully this patch
pyinvoke/invoke#661
in the requirements.txt so we re-enable the
test pipeline step for e2e tests on win

* add intend and cosmetics to header

* add comment to describe patch replace to test/requirements.txt
@ralphie0112358
Copy link

I'd love to have this merged and released! Any updates???

@gbansaghi
Copy link

I would argue that wait() with no timeout would be the correct approach. In practice, 3 seconds should be enough, but the library shouldn't rely on such an arbitrary timeout value (what if it's run on a very old machine, what if there's really heavy I/O, etc.)

Copy link

@krupan krupan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like if we didn't hard-code the timeout to an arbitrary 3, but this is solving the problem I'm having.

mawillcockson added a commit to mawillcockson/install-keepass that referenced this pull request Jan 24, 2020
Have to wait for this to be merged, or work around the issue.

pyinvoke/invoke#660 (comment)
claims v1.2.0 works

As of right now:
https://giant.gfycat.com/BabyishAggravatingGoldeneye.webm
@bitprophet
Copy link
Member

Thanks for this & the additional reports!

I can't drop Python 2 just yet, and I'm also just uncomfortable with a timeout-driven solution, I'd rather attempt to identify the race condition in play & solve it with a threading event or similar; so I'm probably going to supply my own patch instead of merging this directly (tho I reserve the right to backpedal furiously if that goes nowhere 😂 ). Please follow #660 for details.

@bitprophet bitprophet closed this Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants