Skip to content

add support for os.fork and related functions #4877

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 67 commits into from
Apr 24, 2023

Conversation

itsankitkp
Copy link
Contributor

@itsankitkp itsankitkp commented Apr 15, 2023

Motivation: I was trying to run small python server using gunicorn but it failed giving me error fork is not found in module os. This motivated me to add this feature because a running a web server is big deal (for me, atleast)

This patch add fork via libc and test cases from test_os.py
Output is as follows:

>>>>> import os
>>>>> os.fork()
10057
>>>>> 0

In cpython

>>> import os
>>> os.fork()
10138
0
>>> >>>

Current challenges:
implementation of function register_at_fork
I am not entirely sure how to translate cpython's os_register_at_fork_impl to rustpython (suggestions are strongly welcome).

@itsankitkp itsankitkp changed the title add support for os fork and related functions add support for os.fork and related functions Apr 15, 2023
@@ -1127,6 +1127,11 @@ pub(super) mod _os {
OutputMode::String.process_path(curdir_inner(vm)?, vm)
}

#[pyfunction]
fn fork(vm: &VirtualMachine) -> PyObjectRef {
unsafe { vm.ctx.new_int(libc::fork()).into() }
Copy link
Member

@DimitrisJim DimitrisJim Apr 16, 2023

Choose a reason for hiding this comment

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

So, slightly looking into this it seems that you'll need to do a couple things:

  1. Add the register_at_fork function. This does seem to be slightly straight-forward, logic wise at lease. You take in three callables and, after some sanity checks, store them in the interpreter state (typically VirtualMachine's PyGlobalState in our case). This can ideally be a struct but using a vec a-la atexit_funcs could be another approach if the struct is stopping you.
  2. Modify fork to use these. Looking at CPython's implementation this again seems relatively straight-forward. Call before before you issue the fork call and based on if you're the child/parent (pid == 0) issue either after_in_child/after_in_parent.

This is really a very high level overview since I do not have the time to spare. Most likely some other thing might also need to be done and I'm just not seeing it now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, Point 2 looks easy but I was stuck point 1, but now I can move ahead with this pointer.

@itsankitkp
Copy link
Contributor Author

Okay, this is functional now, code snippet:

import os
def before():
   print("before")

def parent():
   print("parent")
def child():
   print("Child")

os.register_at_fork(before=before, after_in_child=child, after_in_parent=parent)
os.fork()

in rustpython

>>>>> import os
>>>>> def before():
.....   print("before")
.....
>>>>> def parent():
.....   print("parent")
.....
>>>>> def child():
.....   print("Child")
.....
>>>>> os.register_at_fork(before=before, after_in_child=child, after_in_parent=parent)
>>>>> os.fork()
before
parent
26312
>>>>> Child
0

In cpython ( 3.12.0a7+)

>>> import os
>>> def child():
...     print("child")
...
>>> def parent():
...     print("parent")
...
>>> def before():
...     print("before")
...
>>> os.register_at_fork(before=before, after_in_child=child, after_in_parent=parent)
>>> os.fork()
before
parent
26349
child
>>> 0

wrt to error handling:

rustpython

>>>>> def before():
.....   print(x)
.....
>>>>> import os
>>>>> os.register_at_fork(before=before)
>>>>> os.fork()
Exception ignored in: <function before at 0x558949c1cbe0>
  File "<stdin>", line 1, in <module>
NameError: name 'x' is not defined
33503
>>>>> 0

cpython

>>> import os
>>> def before():
...     print(x)
...
>>> os.register_at_fork(before=before)
>>> os.fork()
Exception ignored in: <function before at 0x7f4dbf33c260>
Traceback (most recent call last):
  File "<stdin>", line 2, in before
NameError: name 'x' is not defined
29846
0

Some obvious review changes are needed for:

  1. register_at_fork code
  2. Obvious issues
  3. Fix for weird ps (>>)
  4. Random warning (warning: value assigned to pid is never read, I am using this)

Pending: test cases

  1. test_fork_warns_when_non_python_thread_exists needs _testcapi
  2. Unexpected successes
UNEXPECTED SUCCESS: test_waitpid (test.test_os.PidTests.test_waitpid)
UNEXPECTED SUCCESS: test_waitstatus_to_exitcode (test.test_os.PidTests.test_waitstatus_to_exitcode)
UNEXPECTED SUCCESS: test_waitstatus_to_exitcode_kill (test.test_os.PidTests.test_waitstatus_to_exitcode_kill)

in test_waitpid, it shows # TODO: RUSTPYTHON (AttributeError: module 'os' has no attribute 'spawnv')
but spawnv exists in os (not sure what I am missing, will check this).

@itsankitkp itsankitkp marked this pull request as ready for review April 16, 2023 16:20
@itsankitkp itsankitkp closed this Apr 16, 2023
@itsankitkp itsankitkp reopened this Apr 16, 2023
@itsankitkp itsankitkp requested a review from DimitrisJim April 16, 2023 16:22
@DimitrisJim
Copy link
Member

DimitrisJim commented Apr 16, 2023

Nice start! Some initial things so we get a better picture.

  1. Remember to run cargo fmt and clippy to get some easy formatting/lint issues out of the way.
  2. Many of the failures are due to the fact that fork does not exist on that platform. You'll need to restrict it to platforms that do with some cfgs (go for a #[cfg(unix)] for starters)
  3. If tests are passing, we'll need to mark them as failing only for the platforms that won't have fork (currently, this means don't run them on windows, I remember there was a helper added for this specifically recently, see expectedFailureIfWindows and Add unittset.expectedFailureIf for RustPython  #4597 for usages of it).

@fanninpm
Copy link
Contributor

I suspect something in test_httpservers.py might be hanging.

itsankitkp and others added 4 commits April 23, 2023 22:07
Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>
Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>
Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>
@itsankitkp
Copy link
Contributor Author

The error is:

Thanks for looking this up. I knew that would be error but couldn't see it and hence couldn't add skip tag.

The error seems to be hidden probably because it is printed in forked process.

Yes, but I don't know why it didn't got captured at output

while True:
                try:
                    chunk = os.read(fd, 3000)
                except OSError:  # Assume EIO
                    break
                if not chunk:
                    break

I even added sleep(60) at child so that I could send all chunk of traceback. Probably, just UI issue on github action as it worked fine for you.

@itsankitkp
Copy link
Contributor Author

I have made required changes (adding missing XXX RustPython and using #[derive(FromArgs)])
I used match and optional arg but still I couldn't found clean way to raise type error if none of arg are matched (although cpython didn't had test case for that so opened PR python/cpython#103725 so that's win)

@itsankitkp itsankitkp requested a review from youknowone April 23, 2023 19:06
@unittest.skipIf(_testcapi is None, 'needs _testcapi')
@unittest.skipUnless(sys.platform in ("linux", "darwin"),
"Only Linux and macOS detect this today.")
def test_fork_warns_when_non_python_thread_exists(self):
Copy link
Member

Choose a reason for hiding this comment

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

is this originated from CPython source code?

Copy link
Contributor Author

@itsankitkp itsankitkp Apr 24, 2023

Choose a reason for hiding this comment

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

Yes

    @unittest.skipUnless(sys.platform in ("linux", "darwin"),
                         "Only Linux and macOS detect this today.")
    def test_fork_warns_when_non_python_thread_exists(self):
       ...

Copy link
Member

Choose a reason for hiding this comment

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

Ah, got it. it came from later then 3.11.


#[cfg(unix)]
fn py_os_before_fork(vm: &VirtualMachine) -> PyResult<()> {
let before_forkers: Vec<PyObjectRef> = std::mem::take(&mut *vm.state.before_forkers.lock());
Copy link
Member

Choose a reason for hiding this comment

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

Is this not clone but take? So that second run of fork doesn't be affect by it?

Copy link
Contributor Author

@itsankitkp itsankitkp Apr 24, 2023

Choose a reason for hiding this comment

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

Actually you are right.

import os
def a():
   print('a')
def b():
   print('b')
def c():
   os.register_at_fork(before=a)
   os.fork()
   os.register_at_fork(before=b)
   os.fork()
c()

yields

a
b
a
b
a

in cpython. I have remove take operation and now rustpython as same results.
This should be a test case in cpython.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cpython PR: python/cpython#103759

itsankitkp and others added 4 commits April 24, 2023 18:39
Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>
Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>
@itsankitkp
Copy link
Contributor Author

Made required changes, optional arg is working fine as well.

os.register_at_fork(a=8, before=print)

working without any issue

@itsankitkp itsankitkp requested a review from youknowone April 24, 2023 15:07
Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>
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.

Thank you! You made a big step on fork.
I wonder how many more features needed for gunicorn.

@itsankitkp
Copy link
Contributor Author

Thank you! You made a big step on fork. I wonder how many more features needed for gunicorn.

Thanks a lot for review and helping me with this PR.
As you could have guessed, it now fails with AttributeError: 'RLock' object has no attribute '_at_fork_reinit' and few more errors.
Its would need some more work to get it running.

@youknowone youknowone merged commit 322aa68 into RustPython:main Apr 24, 2023
@DimitrisJim DimitrisJim mentioned this pull request May 20, 2023
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.

4 participants