Skip to content

bpo-36668: FIX reuse semaphore tracker for child processes #5172

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

Merged
merged 12 commits into from
Apr 24, 2019

Conversation

tomMoral
Copy link
Contributor

@tomMoral tomMoral commented Jan 13, 2018

The current implementation of the semaphore_tracker creates a new process for each children.
This PR intends to fix this behavior.

The easy fix would be to pass the _pid to the children but the current mechanism to check if the semaphore_tracker is alive relies on waitpid which cannot be used in child processes (the semaphore_tracker is only a sibling of these processes). The main issue is to have a reliable check that either:

  • The pipe is open. This is what is done here by sending a message. I don't know if there is a more efficient way to check it.
  • Check that a given pid is alive. As we cannot rely on waitpid, I don't see an efficient mechanism.

I took the approach of adding a PROBE command in the semaphore tracker. When the pipe is closed, the send command fails and this means the semaphore tracker is down.

https://bugs.python.org/issue36668

@tomMoral tomMoral force-pushed the PR_fix_semtracker branch from 20a6b06 to 0a576a3 Compare April 6, 2018 09:06
@tomMoral tomMoral force-pushed the PR_fix_semtracker branch from 0a576a3 to d9b23e1 Compare March 15, 2019 09:11
pierreglaser added a commit to pierreglaser/loky that referenced this pull request Mar 20, 2019
pierreglaser added a commit to pierreglaser/loky that referenced this pull request Mar 20, 2019
@tomMoral
Copy link
Contributor Author

@pitrou This PR avoid spawning semaphore_tracker in each subprocesses.

@tomMoral tomMoral changed the title bpo-31310: FIX reuse semaphore tracker for child processes bpo-36668: FIX reuse semaphore tracker for child processes Apr 19, 2019
@tomMoral
Copy link
Contributor Author

@pitrou I think this PR is ready

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me. Just a single comment.

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

+1 from me

@pitrou
Copy link
Member

pitrou commented Apr 24, 2019

@tomMoral Can you force-push to trigger an AppVeyor build?

@tomMoral
Copy link
Contributor Author

Done

@pitrou pitrou merged commit 004b93e into python:master Apr 24, 2019
@bedevere-bot
Copy link

@pitrou: Please replace # with GH- in the commit message next time. Thanks!

@pitrou
Copy link
Member

pitrou commented Apr 24, 2019

Thanks @tomMoral . I don't think this needs to be backported to 3.7, do you concur?

@tomMoral
Copy link
Contributor Author

tomMoral commented May 7, 2019

@pitrou yes I agree this is not critical so it is not necessary to backport to 3.7.

@tomMoral tomMoral deleted the PR_fix_semtracker branch May 7, 2019 12:43
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