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

refactor: remove child ownership from Exit filter #207

Merged
merged 5 commits into from
Oct 18, 2024

Conversation

rogercoll
Copy link
Contributor

These change loose the BSD systems API when creating an Exit struct. At the moment, the Exit struct takes ownership of the Child struct and not publicly exporting it, preventing being able to call Child's methods.

The Exit struct is primarily used to wait for a child process termination using the reactor's registration functionality. The Registration uses the polling crate to perform the async wait. The polling only requires a PID, thus these changes removes the child ownership from the package in favor of just providing a PID number.

This will ease the development of the async-process for BSD systems: smol-rs/async-process#49 (comment)

@rogercoll
Copy link
Contributor Author

Any clue why the CI is failing?

   /home/runner/.cargo/bin/cargo audit --json --file ./Cargo.lock
  error: not found: Couldn't load ./Cargo.lock
  Caused by:
    -> I/O operation failed: I/O operation failed: entity not found

Does it require the Cargo.lock file from now on?

src/os/kqueue.rs Outdated

impl Exit {
/// Create a new `Exit` object.
pub fn new(child: Child) -> Self {
Self(Some(child))
pub fn new(pid: NonZeroI32) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to avoid a breaking change, can we make this a new method instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, a new from_pid constructor added in 4308464 instead. I made it unsafe to propagate the safety statement when creating a polling process from a pid.

Wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it actually needs to be unsafe? While it's unsafe for FDs, PIDs are allowed to be racy IIRC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main reason behind the unsafe was to propagate the "safety" statement defined in the polling process (and used by the reactor):

/// Create a `Process` from a PID.
    ///
    /// # Safety
    ///
    /// The PID must be tied to an actual child process.
    pub unsafe fn from_pid(pid: std::num::NonZeroI32, ops: ProcessOps) -> Self {
        Self {
            pid: unsafe { rustix::process::Pid::from_raw_unchecked(pid.get()) },
            ops,
            _lt: PhantomData,
        }
    }

With the new Exit status API, the struct can be crated for any given u32 value, thus the possibility of not being tied to an actual child process (differently than child.id()). This is the error when the provided pid is not attached to a process:

Error: Os { code: 3, kind: Uncategorized, message: "No such process" }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that with the new from_pid it lets the user provide any PID value, do you still think the unsafe guidance is not needed?

Thanks for your feedback.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, keep it as unsafe for now. It's been a while since I've reviewed kqueue's implementation, I trust my past self more in that regards.

Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

LGTM aside from one minor nitpick

Comment on lines 30 to 35
let process = Filter::new(Exit::new(process))?;
let process_handle = unsafe {
Filter::new(Exit::from_pid(
NonZeroI32::new(process.id().try_into().expect("invalid process pid"))
.expect("non zero pid"),
))?
};
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer keeping the example simple.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sgtm 610773a

@notgull notgull merged commit 9713083 into smol-rs:master Oct 18, 2024
20 of 21 checks passed
@notgull
Copy link
Member

notgull commented Oct 18, 2024

I'll try to push this as a release this weekend

@rogercoll rogercoll deleted the remove_child_ownership branch October 18, 2024 05:59
@rogercoll
Copy link
Contributor Author

@notgull Is there any ETA for the next release? Thanks!

@notgull
Copy link
Member

notgull commented Nov 4, 2024

Sorry, I got distracted. Assuming nothing major comes up tonight I'll push a release.

@notgull
Copy link
Member

notgull commented Nov 5, 2024

...I said, blissfully unaware that something major was about to come up. 😵‍💫

I'll release this ASAP

@notgull notgull mentioned this pull request Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants