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

[stdlib] Adds functionality to spawn and manage processes from exec. file #3998

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

izo0x90
Copy link

@izo0x90 izo0x90 commented Feb 14, 2025

Foundation for this PR is set here, it adds the needed lowlevel utilities:

  • Adds vfork, execvp, kill system call utils. to Mojos cLib binds
  • Adds read_bytes to file descriptor

Once that PR is merged the only changes left on this one are the ones related to creating the os.process module.

  • Adds utility struct. to allow spawning and managing child processes
    from an executable file
    • run factory method that creates process and returns "management"
      object
    • Instance methods to send signals to child process, INT, HUP, KILL

Ex.:

import os

fn main():
 p = os.Process.run("ls", List[String]("-lha"))
 p. interrupt()

@izo0x90 izo0x90 changed the title [] Adds functionality to spawn and manage processes from exec. file [stdlib] Adds functionality to spawn and manage processes from exec. file Feb 14, 2025
Copy link
Contributor

@owenhilyard owenhilyard left a comment

Choose a reason for hiding this comment

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

Good work so far, just a few fixes.


def main():
test_process_run()
# TODO: How can exception being raised on missing file be asserted
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

The issue here from what I understand seems to be that I am rasing the exception in the forked child process when we don't find the executable file ... which try actually handles gracefully ... so this works fine:

try:
  _ = Process.run("file_does_not_exist", List[String]())
except e:
 print(e)

But for some reason (I assume related to the raise being in a different process) when try to validate the raise with:

with assert_raise():
  _ = Process.run("file_does_not_exist", List[String]())

It crashed the test suite with a no such process error. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that raises in a fork pre-exec is unsound, you'll need to check before you fork.

Copy link
Author

Choose a reason for hiding this comment

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

Ok will work on something to do an explicit check before the the fork happens.

Copy link
Author

Choose a reason for hiding this comment

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

Update: Did a little digging on the established/ canonical way to deal with this on posix/ unix systems and implemented it here, everything is working on Macos ... just running into some issues with linux, guessing some inconsistencies between the platform libc calls, working on clearing that up.

Copy link
Author

Choose a reason for hiding this comment

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

@owenhilyard Hey quick question for you, so I did some testing in a linux container. The crashing/ failing tests seem to be related to the test runners on Linux and not the actual functionality. As you can see I can successfully run a process on linux using the Process utilities ( see screen shot below ).

Do you have any ideas on how to proceed from here, it seems the code being added is functioning as intended ? Thanks again for all the feedback/ help! 🔥

Screenshot 2025-02-20 at 7 08 42 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Does doing non-trival work in the other threads function correctly? Try to do something like launch a simple max graph from one of them.

Copy link
Author

@izo0x90 izo0x90 Feb 21, 2025

Choose a reason for hiding this comment

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

Did some further testing now that I got a min, what appears to be failing on Linux is as follows:

  • We issue an execvp call to a non existent executable in the forked process
  • The forked process signals to the parent through the shared pipe that there was an error
  • Forked process closes out pipe and exits
  • Process.run function finished running
  • Main process that called Process.run finishes running, all the library and user code finishes running
  • At the end when that process is exiting on Linux, we get a fatal error from the Mojo runtime
    Crash at program exit:
Screenshot 2025-02-21 at 4 21 31 PM - However if we just call `exit()` at the end of our program that called `Process.run` everything finishes closing out the program fine and we get no errors from the runtime 🫤

So literally this causes the runtime to complain and error out:

from collections import List
from os import Process
from sys._libc import exit
fn main() raises:
    print("IN MAIN")
    try:
        print("ATTEMPTING EXECUTION")
        _ = Process.run(
            "brokeenn", List[String]("\n\nCORRECTLY RUNNING CHILD PROCESS !")
        )
    except:
        print("GOT THE EXCEPTION")
    print("END MAIN PROCESS")
    print("IF WE CALL `exit()` NO ISSUE. IF WE DO NOT, RUNTIME CRASHES.")
    # exit(1) # NO CALL TO `exit`, WE GET AN ERROR FROM THE RUNTIME

But just adding exit({any_value}) allows everything to run exit the program fine:

from collections import List
from os import Process
from sys._libc import exit
fn main() raises:
    print("IN MAIN")
    try:
        print("ATTEMPTING EXECUTION")
        _ = Process.run(
            "brokeenn", List[String]("\n\nCORRECTLY RUNNING CHILD PROCESS !")
        )
    except:
        print("GOT THE EXCEPTION")
    print("END MAIN PROCESS")
    print("IF WE CALL `exit()` NO ISSUE. IF WE DO NOT, RUNTIME CRASHES.")
    exit(1) # WITH A CALL TO EXIT, EVERYTHING FINISHES RUNNING FINE

We can observe this issue with:

  • mojo run
  • mojo build and then run the binary
  • In the test being ran by the test suite
    Always once all stdlib and user code has finished running and the runtime is doing cleanup closing out things.

Wonder what next steps can be since we don't have any access to the Mojo runtime codebase ?

@owenhilyard @JoeLoser (Again thanks for the feedback and help!)

@izo0x90 izo0x90 force-pushed the Hristo/Add-basic-process-exec-facilities branch 4 times, most recently from 63a5a80 to 294413f Compare February 17, 2025 17:09
@JoeLoser
Copy link
Collaborator

Exciting to see work on this area! Feel free to ping me when CI is passing for Linux as well - we'll need to ensure everything works on both Linux and macOS before we can land this.

@JoeLoser JoeLoser self-assigned this Feb 21, 2025
@ConnorGray
Copy link
Collaborator

Hi @izo0x90, thanks for working on this, this adds a lot of great new APIs! 🔥 🙂

The team just had some discussion about this PR and are generally looking forward to merging this functionality 🙂

I did want to ask if you might be able to split this PR, to make each individual component added easier to review.

For example, you might start with a smaller PR that just contains the new _libc.mojo functions and the FileDescriptor.read_bytes() method on their own. That would make it easier to read and provide meaningful suggestions for those changes on their own. Then this PR could stay focused to meaty new process.mojo file being added here.

Please let me know if you think splitting this up could be feasible, and if I can provide any help with that just let me know :)

Thanks again for contributing to Mojo, we're glad you're hear 😄

  - Add `read_bytes` capability to `FileDecscriptor`
  - Add file descriptor controls function to Libc. bindings
  - Adds vfork, execvp, kill system call utils. to Mojos cLib binds

Signed-off-by: Hristo I. Gueorguiev <[email protected]>
Update `read_bytes` from Span[Byte] to List[Byte] for better memory
safety and "user ergonomics"

Signed-off-by: Hristo I. Gueorguiev <[email protected]>
@izo0x90 izo0x90 force-pushed the Hristo/Add-basic-process-exec-facilities branch from 294413f to fd929b0 Compare February 25, 2025 15:51
@izo0x90
Copy link
Author

izo0x90 commented Feb 25, 2025

@ConnorGray thank you for all the feedback, excited to be working on this!! 😄

I updated this PR with the changes and also split off the foundational changes into their own PR Hristo/foundations for os process module.

This PR is now rebased off the branch for the foundational PR and all those on here diff will be gone once it gets merged.

Let me know if there are any additional changes that need to be made here or on the foundational PR.

Also the only issue with the Process PR that i am still running into is the strange fail states on Linux when the runtime is cleaning up/ exiting, if you have any insights or feedback on that as well it would be much much appreciated.

Thanks!!! 🔥

Update the list creation to conform to upcoming List API changes that
make the list size a `private attribute`

Signed-off-by: Hristo I. Gueorguiev <[email protected]>
@izo0x90 izo0x90 force-pushed the Hristo/Add-basic-process-exec-facilities branch from fd929b0 to 0ce641a Compare February 25, 2025 16:56
izo0x90 and others added 9 commits February 26, 2025 11:14
Co-authored-by: Connor Gray <[email protected]>
Signed-off-by: Hristo (Izo) G. <[email protected]>
Co-authored-by: Connor Gray <[email protected]>
Signed-off-by: Hristo (Izo) G. <[email protected]>
Co-authored-by: Connor Gray <[email protected]>
Signed-off-by: Hristo (Izo) G. <[email protected]>
Co-authored-by: Connor Gray <[email protected]>
Signed-off-by: Hristo (Izo) G. <[email protected]>
Co-authored-by: Connor Gray <[email protected]>
Signed-off-by: Hristo (Izo) G. <[email protected]>
Co-authored-by: Connor Gray <[email protected]>
Signed-off-by: Hristo (Izo) G. <[email protected]>
Co-authored-by: Connor Gray <[email protected]>
Signed-off-by: Hristo (Izo) G. <[email protected]>
- `read_bytes` takes in Span[Byte] buffer as opposed to allocating
  to avoid n+1 alloc. issues etc.

- Remove c_str_ptr type from ffi module since it is not a convention set
  by C lang., it will potentially get added back in at a later date
  after discussions and under its own PR

- Doc string updates to libC funcs.

Co-authored-by: Connor Gray <[email protected]>
Signed-off-by: Hristo (Izo) G. <[email protected]>
- Adds fork, execvp, kill system call utils. to Mojos cLib binds

- Adds utility struct. to allow spawning and managing child processes
  from an executable file
  - `run` factory method that creates process and returns "management"
    object
  - Instance methods to send signals to child process, INT, HUP, KIL

Signed-off-by: Hristo I. Gueorguiev <[email protected]>
- Move `process` related code to its own file
- Use `vfork` to avoid issues with threading until they are resolved in
  Mojo
- Sending signals to process returns success status
- Docstr additions

Signed-off-by: Hristo I. Gueorguiev <[email protected]>
Implements canonical fork/ exec error signaling process that uses a pipe
configure to close on `exec` sys. call to notify the parent process if
an error occurs in the child and it is unable to `exec` the desired
executable file

- Implement Pipe abstraction
- Add `read_bytes` capability to `FileDecscriptor`
- Add file descriptor controls function to Libc. bindings
- Fix previously broken test for rasing on exec errors

Refs:
- https://cr.yp.to/docs/selfpipe.html

Signed-off-by: Hristo I. Gueorguiev <[email protected]>
- `read_bytes` takes in already alloced. buffer to avoid n+1 issues in
  loops etc.

Signed-off-by: Hristo I. Gueorguiev <[email protected]>
@izo0x90 izo0x90 force-pushed the Hristo/Add-basic-process-exec-facilities branch from 0ce641a to c38e95d Compare February 26, 2025 19:39
@izo0x90 izo0x90 requested a review from a team as a code owner February 26, 2025 19:39
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