Use `os.execvp` for `poetry run` by pbzweihander · Pull Request #1236 · python-poetry/poetry

pbzweihander · 2019-07-19T02:57:27Z

Added tests for changed code.
Updated documentation for changed code.

This resolves #993.

poetry run uses subprocess.call to run the other program, making poetry spawn a new child process.
The problem is when SIGTERM is handed over; poetry does not pass it to the child process, instead terminates itself and makes zombie processes.
So I changed poetry run to use os.execvp, so poetry will replace itself with a new process and properly handle signals.

Test

I made this small script in poetry/tester.py:

import signal
import time


def handler(signum, frame):
    print(f"signal {signum} received")


def main():
    print("start")
    signal.signal(signal.SIGTERM, handler)
    time.sleep(10)
    print("end")


if __name__ == "__main__":
    main()

Then ran poetry run python poetry/tester.py and tried to kill it.

Before

$ poetry run python poetry/tester.py &
[1] 23987
start
$ ps -ax | grep tester
24270 pts/11   S      0:00 python /home/.../poetry run python poetry/tester.py
24350 pts/11   S      0:00 /home/.../python poetry/tester.py
24359 pts/11   S+     0:00 grep --color=auto tester
$ kill -TERM $!
[1]+  Terminated              poetry run python poetry/tester.py
$ ps -ax | grep tester
24350 pts/11   S      0:00 /home/.../python poetry/tester.py
24446 pts/11   S+     0:00 grep --color=auto tester
$ 
end

Fixed

$ python -m poetry run python poetry/tester.py &
[1] 25282
start
$ ps -ax | grep tester
25282 pts/11   S      0:00 /home/.../python poetry/tester.py
25364 pts/11   S+     0:00 grep --color=auto tester
$ kill -TERM $!
signal 15 received
$ 
end
[1]+  Done                    python -m poetry run python poetry/tester.py

Test - scripts feature

I added the following lines to pyprojects.toml.

...
[tools.poetry.scripts]
...
tester = "poetry.tester:main"

Then ran poetry run tester and tried to kill it.

Before

$ poetry run tester &
[1] 27077
start
$ ps -ax | grep tester
27077 pts/11   S      0:00 python /home/.../poetry run tester
27173 pts/11   S      0:00 /home/.../python -c import sys; from importlib import import_module; sys.argv = ['tester']; import_module('poetry.tester').main()
27189 pts/11   S+     0:00 grep --color=auto tester
$ kill -TERM $!
[1]+  Terminated              poetry run tester
$ ps -ax | grep tester
27173 pts/11   S      0:00 /home/.../python -c import sys; from importlib import import_module; sys.argv = ['tester']; import_module('poetry.tester').main()
27266 pts/11   S+     0:00 grep --color=auto tester
$ 
end

Fixed

$ python -m poetry run tester &
[1] 27996
start
$ ps -ax | grep tester
27996 pts/11   S      0:00 /home/.../python -c import sys; from importlib import import_module; sys.argv = ['tester']; import_module('poetry.tester').main()
28082 pts/11   S+     0:00 grep --color=auto tester
$ kill -TERM $!
signal 15 received
$ 
end
[1]+  Done                    python -m poetry run tester

digitalresistor · 2019-07-23T05:21:42Z

👍 on this however using os.execvp does not work on Windows. Please take a look at and steal (my permission is hereby given to relicense it under whatever Poetry is licensed as) my code from here: https://github.com/bertjwregeer/vrun/tree/master/src/vrun which supports both Unix and Windows.

digitalresistor · 2019-07-23T05:22:58Z

specifically you want this file: https://github.com/bertjwregeer/vrun/blob/master/src/vrun/oscompat.py

simnalamburt · 2019-07-24T05:57:58Z

The document says that os.execvp is available in Windows. May I ask what exception is thrown when we try to use os.execvp in Windows?

https://docs.python.org/3/library/os.html#os.execvp

pbzweihander · 2019-07-24T07:02:42Z

I found in Windows, exec does not actually replace current process and instead creates a new one and exits the current one.

Reference:

So I think poetry needs to create a new process and wait until the new one exits. (https://github.com/bertjwregeer/vrun/blob/a726be8106a00621361f60079ac403e5bc16864a/src/vrun/oscompat.py#L34)

digitalresistor · 2019-07-24T07:10:29Z

If you try to use the exec calls on Windows you get into a really weird state where the process you are running can't be controlled from the cmd.exe window it is spawned from.

You need to do the whole popen dance so that you can send key strokes from the foreground to the process that was spawned on Windows.

digitalresistor · 2019-07-24T07:11:53Z

I see that @pbzweihander got there before me! :-D

Please steal my code, I didn't come to that conclusion the easy way!

simnalamburt · 2019-08-08T04:34:30Z

Now I think this PR is ready to be merged since I see no more blockers.

brycedrennan · 2019-08-16T05:03:19Z

Thanks for your contribution. I'm not sure when someone will have a chance to dig into this.

I imagine its pretty hard to add an automated test for this?

digitalresistor · 2019-08-16T17:53:25Z

What kind of tests are you looking for?

maksbotan · 2019-10-04T12:20:44Z

@sdispater so, are there plans to merge this one? :)

sdispater · 2019-10-04T13:34:25Z

Thanks a lot for this PR!

* Use os.execvp for poetry run * Use subprocess.Popen for Windows * Handle env kwargs

rpdelaney · 2019-12-29T20:15:42Z

Does this have any effect on performance?

simnalamburt · 2019-12-30T10:39:44Z

It'll boost performance since os.execvp is faster than subprocess.call

akdor1154 · 2020-01-16T05:58:17Z

I think this broke calling scripts on Windows - if I do poetry run script.py it used to get called as subprocess.call(script.py, shell=True) but now gets called as Popen([script.py]) This doesn't work, see https://stackoverflow.com/questions/912830/using-subprocess-to-run-python-script-on-windows.

simnalamburt · 2020-01-16T07:46:59Z

It’s actually calling Popen([‘python3.exe’, blabla]) which seems OK according to your link https://stackoverflow.com/a/912847

benjirewis · 2023-03-30T19:54:40Z

I've noticed killing poetry processes with SIGTERM creates a status code of 143. While that error code does indicate graceful termination after receiving SIGTERM, why not just return 0?

github-actions · 2024-03-03T22:48:23Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

pbzweihander force-pushed the use-execvp-for-run branch from ae1b0a5 to 69a1ac2 Compare July 24, 2019 11:03

pbzweihander force-pushed the use-execvp-for-run branch from 69a1ac2 to e5ebabe Compare August 1, 2019 06:01

brycedrennan added the kind/feature Feature requests/implementations label Aug 16, 2019

rusty added 3 commits October 2, 2019 16:23

Use os.execvp for poetry run

8d8fbb9

Use subprocess.Popen for Windows

163ed3d

Handle env kwargs

2c467a9

pbzweihander force-pushed the use-execvp-for-run branch from e5ebabe to 2c467a9 Compare October 2, 2019 07:24

sdispater mentioned this pull request Oct 4, 2019

Improve run command #1428

Closed

2 tasks

sdispater changed the base branch from develop to master October 4, 2019 13:33

sdispater merged commit a581ff6 into python-poetry:master Oct 4, 2019

pbzweihander deleted the use-execvp-for-run branch October 4, 2019 15:34

michielboekhoff pushed a commit to michielboekhoff/poetry that referenced this pull request Oct 29, 2019

Use os.execvp for poetry run (python-poetry#1236)

2a9459c

* Use os.execvp for poetry run * Use subprocess.Popen for Windows * Handle env kwargs

michielboekhoff pushed a commit to michielboekhoff/poetry that referenced this pull request Oct 29, 2019

Use os.execvp for poetry run (python-poetry#1236)

dbba557

* Use os.execvp for poetry run * Use subprocess.Popen for Windows * Handle env kwargs

kevincon mentioned this pull request Nov 7, 2020

Fix running console scripts of editable dependencies on Windows #3339

Merged

2 tasks

benjirewis mentioned this pull request Mar 30, 2023

RSDK-2389 Support reconfiguration of modules viamrobotics/rdk#2114

Merged

github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2024

Comments

Conversation

pbzweihander commented Jul 19, 2019 • edited by brycedrennan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test

Before

Fixed

Test - scripts feature

Before

Fixed

Uh oh!

digitalresistor commented Jul 23, 2019

Uh oh!

digitalresistor commented Jul 23, 2019

Uh oh!

simnalamburt commented Jul 24, 2019

Uh oh!

pbzweihander commented Jul 24, 2019

Uh oh!

digitalresistor commented Jul 24, 2019

Uh oh!

digitalresistor commented Jul 24, 2019

Uh oh!

simnalamburt commented Aug 8, 2019

Uh oh!

brycedrennan commented Aug 16, 2019

Uh oh!

digitalresistor commented Aug 16, 2019

Uh oh!

maksbotan commented Oct 4, 2019

Uh oh!

sdispater commented Oct 4, 2019

Uh oh!

rpdelaney commented Dec 29, 2019

Uh oh!

simnalamburt commented Dec 30, 2019

Uh oh!

akdor1154 commented Jan 16, 2020

Uh oh!

simnalamburt commented Jan 16, 2020

Uh oh!

benjirewis commented Mar 30, 2023

Uh oh!

github-actions bot commented Mar 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

pbzweihander commented Jul 19, 2019 •

edited by brycedrennan

Loading