Don't setpgid() on actors #3347
Conversation
| @@ -976,7 +972,7 @@ def _wait_for_and_process_task(self, task): | |||
| driver_id, function_id.id()) == execution_info.max_calls) | |||
| if reached_max_executions: | |||
| self.local_scheduler_client.disconnect() | |||
| os._exit(0) | |||
| sys.exit(0) | |||
There was a problem hiding this comment.
@robertnishihara this doesn't have to be an os._exit right? That prevents exit hooks from running.
There was a problem hiding this comment.
It doesn't need to be anything in particular. Just needs to exit the process.
| @@ -29,12 +29,13 @@ def __init__(self, config): | |||
| self.action_space = Discrete(2) | |||
| self.observation_space = Discrete(2) | |||
| # Subprocess that should be cleaned up | |||
| self.subproc = subprocess.Popen(UNIQUE_CMD, shell=True) | |||
| self.subproc = subprocess.Popen(UNIQUE_CMD.split(" "), shell=False) | |||
There was a problem hiding this comment.
Can't launch in a shell now, since we don't have process group containment. Launching it in a shell causes the process to escape.
|
Test FAILed. |
|
Test FAILed. |
|
You seem to be reverting the code change from #3297 but leaving the test in place, is that right? What's the point of the test if we revert the pgid change? |
|
We're still testing the atexit hooks, just not the auto pgroup killing.
Hence the addition of the kill line to the test.
This is also only reverting a portion of that PR. The important thing is to
not os._exit() which bypasses exit hooks.
…On Sat, Nov 17, 2018, 11:54 PM Robert Nishihara ***@***.***> wrote:
You seem to be reverting the code change from #3297
<#3297> but leaving the test in
place, is that right? What's the point of the test if we revert the pgid
change?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#3347 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAA6SrhCYmvBnBud2PwT2Q21im8lURnuks5uwRIngaJpZM4YnyAs>
.
|
|
Test FAILed. |
ericl
added
the
tests-ok
What do these changes do?
Don't call setpgid() on actors (introduced in #3297)
pros: Ctrl-C won't leak actor processes
cons: actors can more easily leak subprocesses (but you can manage these via atexit hooks)
Related issue number
Closes #3345