Skip to content

os: implement fork #2750

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 2 commits into from
Closed

os: implement fork #2750

wants to merge 2 commits into from

Conversation

yossi-k
Copy link
Contributor

@yossi-k yossi-k commented Jul 24, 2021

This seems to work and tests pass (test_posix:test_register_at_fork).

I took a look at CPython's implementation and it seems they're doing more things before/after fork, namely acquiring the import lock, and re-initiating the interpreter in the child process (see here - https://github.com/python/cpython/blob/main/Modules/posixmodule.c#L593). Do we want to do the same thing here?

@yossi-k yossi-k force-pushed the os-fork branch 3 times, most recently from c1de780 to b94cba7 Compare July 26, 2021 12:41
Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

I personally support hard compatibility with CPython.
At least, leaving comments about CPython implementations for each places will be helpful for future workers.

@youknowone
Copy link
Member

what's happend to the ubuntu tests?

@DimitrisJim
Copy link
Member

Whatever it is it isn't reproducible, as threads tend to be. Locally it once started spinning on test_threading indefinitely while its currently passing.

@yossi-k
Copy link
Contributor Author

yossi-k commented Jul 27, 2021

I’ll take another look. The problem is that fork enables functions like os.spawn which in turn enable a whole bunch of new tests.

@DimitrisJim
Copy link
Member

thanks! This is superseded by #4877 if I'm not mistaken. Feel free to re-open if I'm mistaken.

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.

3 participants