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

Do not use setsid directly, use dedicated start_new_session #155

Merged
merged 2 commits into from
Aug 21, 2024
Merged

Conversation

yarikoptic
Copy link
Member

Just ran into
https://stackoverflow.com/questions/42257512/difference-between-subprocess-popen-preexec-fn-and-start-new-session-in-python which points to python docs at
https://docs.python.org/3.4/library/subprocess.html?highlight=subprocess#popen-constructor

Warning The preexec_fn parameter is not safe to use in the presence
of threads in your application. The child process could deadlock before
exec is called. If you must use it, keep it trivial! Minimize the number of
libraries you call into.

Note If you need to modify the environment for the child use the env
parameter rather than doing it in a preexec_fn. The start_new_session
parameter can take the place of a previously common use of preexec_fn to
call os.setsid() in the child.

So smells like we better use this dedicated option added in 3.2 .

But may be it was considered and argued against @asmacdo and doesn't work as desired

(there is a separate discussion on possibly using group not session ids in #82)

Just ran into
https://stackoverflow.com/questions/42257512/difference-between-subprocess-popen-preexec-fn-and-start-new-session-in-python
which points to python docs at
https://docs.python.org/3.4/library/subprocess.html?highlight=subprocess#popen-constructor

    Warning The preexec_fn parameter is not safe to use in the presence
    of threads in your application. The child process could deadlock before
    exec is called. If you must use it, keep it trivial! Minimize the number of
    libraries you call into.

    Note If you need to modify the environment for the child use the env
    parameter rather than doing it in a preexec_fn. The start_new_session
    parameter can take the place of a previously common use of preexec_fn to
    call os.setsid() in the child.

So smells like we better use this dedicated option added in 3.2 .
Copy link

codecov bot commented Aug 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.56%. Comparing base (ebf7b17) to head (7f78031).
Report is 7 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #155   +/-   ##
=======================================
  Coverage   95.56%   95.56%           
=======================================
  Files           2        2           
  Lines         451      451           
  Branches       69       69           
=======================================
  Hits          431      431           
  Misses         10       10           
  Partials       10       10           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@asmacdo
Copy link
Member

asmacdo commented Aug 21, 2024

I don't think this was considered, looks like an improvement to me. I'm curious if start_new_session is actually slower, or if its just random flake.

For the failing test I started that requirement as tight as possible, opting to relaxed that requirement only as needed, which I've done in latest push.

@asmacdo asmacdo merged commit cb71d9c into main Aug 21, 2024
30 checks passed
@asmacdo asmacdo deleted the rf-setsid branch August 21, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants