Support creating an Async with AsFd rather than AsRawFd

[joshtriplett]

This adds support for creating an Async on a type that supports AsFd
(AsSocket on Windows), without requiring the type to implement AsRawFd
(AsRawSocket on Windows).

This builds on #135 . The first commit
is the same as that PR.

The second commit factors out an internal helper that contains most of the
logic of the Async::new constructor: setting the fd or socket to
non-blocking.

The third commit then adds the new constructor; using the helper, these are
only a few lines (plus the doc comments).

[notgull]

I can't speak for the other admins, but my preference at this point would be to just have a breaking change here rather than adding a new method. It's debatably unsafe to keep using AsRawFd at this point, and I'd rather have that other another hard to maintain constructor.

[joshtriplett]

@notgull That works for me. Happy to rework this into a major-version change instead.

[joshtriplett]

@notgull Done.

Note that this doesn't currently build because inotify and timerfd don't currently implement AsFd. It looks like inotify has a whole FdGuard type that could largely be replaced with OwnedFd.

[joshtriplett]

timerfd PR: https://github.com/main--/rust-timerfd/pull/13

[joshtriplett]

inotify PR: hannobraun/inotify-rs#202

[notgull]

I should probably ask. @smol-rs/admins Are we fine with a breaking change at this point? Note that Async is exposed in the smol crate, so pushing this would mean releasing smol v2.0.0.

[zeenix]

I should probably ask. @smol-rs/admins Are we fine with a breaking change at this point? Note that Async is exposed in the smol crate, so pushing this would mean releasing smol v2.0.0.

Sure. However, I think most users out there will just keep using 1.0 w/o realizing that 1.x is no longer supported. The fix is super trivial though so no sure if it matters at all. 🤷

so tl'dr go for it. :)

[notgull]

No real objections now that Debian Bookworm is out. Once the errors are fixed we should be good to go.

[joshtriplett]

I've rebased the current version, and updated to use a version of timerfd that builds with this change.

[joshtriplett]

FWIW, I don't know if a breaking change is the right approach here, either. That'd be required to change the new function to use AsFd, but the original approach I used here would just create a different constructor for the AsFd case. That wouldn't let us jettison RawFd entirely, but it'd let people entirely ignore RawFd. We could even, if we wanted, deprecate the RawFd-based constructor to encourage people to migrate, without having to make a breaking change.

Ultimately it's up to you, and I'll adapt this PR to whatever you'd like to do; I just don't want to give the impression of pushing for a breaking change, or being enthusiastic about one. :)

[taiki-e]

There are some breaking changes we would like to make to some of the other smol-rs crates (re-export in futures-lite, seal ext traits in async-fs, default features in async-lock, etc.), so I am not opposed to releasing a smol 2.0 that includes them along with this change.

That said, we need to consider the downstream impact of them. For example, async-std uses some smol-rs crates types as-is as public APIs, so a major version bump may be required for async-std to update to use the latest versions of them.

[notgull]

FWIW, I don't know if a breaking change is the right approach here, either. That'd be required to change the new function to use AsFd, but the original approach I used here would just create a different constructor for the AsFd case. That wouldn't let us jettison RawFd entirely, but it'd let people entirely ignore RawFd. We could even, if we wanted, deprecate the RawFd-based constructor to encourage people to migrate, without having to make a breaking change.

I would be opposed to keeping around a constructor for RawFd usage, unless it is explicitly marked as unsafe. We should be enforcing I/O safety by default.

[zeenix]

I would be opposed to keeping around a constructor for RawFd usage, unless it is explicitly marked as unsafe. We should be enforcing I/O safety by default.

AFAIK it's not considered a breaking change to mark an unsafe function as such so we should do that regardless (if the raw constructor is kept).

[notgull]

AFAIK it's not considered a breaking change to mark an unsafe function as such so we should do that regardless (if the raw constructor is kept).

It feel unidiomatic to have an unsafe new function and a safe new_with_as_fd function. If the consensus is to do that I would follow along, but we've been at 1.0 for about 3 years now and a breaking change wouldn't be too bad.

[notgull]

There are some breaking changes we would like to make to some of the other smol-rs crates (re-export in futures-lite, seal ext traits in async-fs, default features in async-lock, etc.), so I am not opposed to releasing a smol 2.0 that includes them along with this change.

That said, we need to consider the downstream impact of them. For example, async-std uses some smol-rs crates types as-is as public APIs, so a major version bump may be required for async-std to update to use the latest versions of them.

I've created an issue on the main repo for further discussion of this, see smol-rs/smol#258

[joshtriplett]

AFAIK it's not considered a breaking change to mark an unsafe function as such so we should do that regardless (if the raw constructor is kept).

As far as I know, that's a breaking change; existing code won't compile anymore.

[zeenix]

AFAIK it's not considered a breaking change to mark an unsafe function as such so we should do that regardless (if the raw constructor is kept).

As far as I know, that's a breaking change; existing code won't compile anymore.

True but my understanding is that since it notifies user of potential dangers (that they may not be aware of before), it is not considered a semver break by convention. Neither the relevant RFC nor Cargo references mention this explicitly though. 🤷

[notgull]

Superseded by #142