-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
vm/src/stdlib/os.rs
Outdated
@@ -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() } |
There was a problem hiding this comment.
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:
- 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 (typicallyVirtualMachine
'sPyGlobalState
in our case). This can ideally be a struct but using a vec a-laatexit_funcs
could be another approach if thestruct
is stopping you. - Modify
fork
to use these. Looking at CPython's implementation this again seems relatively straight-forward. Callbefore
before you issue thefork
call and based on if you're the child/parent (pid == 0
) issue eitherafter_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.
There was a problem hiding this comment.
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.
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
In cpython ( 3.12.0a7+)
wrt to error handling: rustpython
cpython
Some obvious review changes are needed for:
Pending: test cases
in test_waitpid, it shows # TODO: RUSTPYTHON (AttributeError: module 'os' has no attribute 'spawnv') |
Nice start! Some initial things so we get a better picture.
|
Co-authored-by: fanninpm <fanninpm@miamioh.edu>
I suspect something in |
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>
Thanks for looking this up. I knew that would be error but couldn't see it and hence couldn't add skip tag.
Yes, but I don't know why it didn't got captured at output
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. |
I have made required changes (adding missing XXX RustPython and using #[derive(FromArgs)]) |
@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): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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):
...
There was a problem hiding this comment.
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.
vm/src/stdlib/os.rs
Outdated
|
||
#[cfg(unix)] | ||
fn py_os_before_fork(vm: &VirtualMachine) -> PyResult<()> { | ||
let before_forkers: Vec<PyObjectRef> = std::mem::take(&mut *vm.state.before_forkers.lock()); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>
Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>
Made required changes, optional arg is working fine as well.
working without any issue |
Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>
There was a problem hiding this 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.
Thanks a lot for review and helping me with this PR. |
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:
In cpython
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).