polachok / fahrenheit

toy futures executor 🚒📖🔥
Other
233 stars 23 forks source link

Removing unsafe by replacing libc with nix #15

Open yoshuawuyts opened 5 years ago

yoshuawuyts commented 5 years ago

Hi!

First off: thanks so much for making this project! I've learned a lot just by reading it, which is fantastic!

Observation

I was going through the source today (on the futures-0.3 branch actually :sparkles:) and something that stood out to me is that there's quite some unsafe sprinkled throughout the code to interact with the OS through libc. nix is a package which aims to make using libc's functionality safe, removing the need for unsafe.

I feel this would have a few benefits:

Example

For example the select(2) loop in the reactor currently looks like this:

let rv = unsafe {
    select(
        nfds,
        &mut read_fds,
        &mut write_fds,
        std::ptr::null_mut(),
        &mut tv,
    )
};

// don't care for errors
if rv == -1 {
    panic!("select()");
} else if rv == 0 {
    debug!("timeout");
} else {
    debug!("data available on {} fds", rv);
}

Using the nix select function this could be rewritten to something like:

// don't care for errors
let rv = select(
    Some(nfds),
    Some(&mut read_fds),
    Some(&mut write_fds),
    None,
    Some(&mut tv),
).unwrap();

debug!("data available on {} fds", rv);

Which might feel a bit more natural for most Rustaceans.

Conclusion

We made a case for replacing the usage of libc with the nix crate, citing potential benefits, and created an example conversion using existing code. I hope this issue might come in helpful. Thanks for your time!

polachok commented 5 years ago

Hi,

I'm happy the project was useful for you!

Now to the point, I actually considered using nix, but decided against it for two reasons:

1.minimal dependencies I think, in a teaching project, it makes sense to minimize dependencies, as every dependency adds cognitive load. A reader may be unfamiliar with nix, so they would have to consult nix docs. On the other hand, libc::select makes it clear that this is a well known C select function.

2.nix being huge Nix covers a lot of APIs and consists of ~20k lines + 4 dependencies. Well, even mio dropped nix because it's heavy.

So, considering this, what do you think about adding a safe wrapper around select to fahrenheit itself? It would look as safe as nix in the main file, but a curious reader could inspect it in some util.rs right here.