Bump polling and socket2 version to latest

[irvingoujAtDevolution]

changed code accrodingly, we may need to pay extra attention and discuss on when to remove socket from poller as suggested by polling's documentation

[keepsimple1]

Thanks for the PR. Unfortunately bringing in unsafe in this case is a big no-no.

[CBenoit]

Thanks for the PR. Unfortunately bringing in unsafe in this case is a big no-no.

It’s good to be cautious about introducing unsafe blocks, but there's a compelling reason to reconsider in this instance.

The need for unsafe arises from the Poller::poll function in the polling crate being inherently "I/O unsafe", a characteristic that is now explicitly indicated in the function signature and documentation with the release of polling 3.x.

This change, although a significant shift, simply makes explicit what was previously an implicit requirement: the necessity for the RawFd to be valid as long as it’s held by the Poller.
You can check the PR for more details: smol-rs/polling#123

The I/O safety RFC is also explicitly documenting that such functions should be marked as unsafe:

Functions accepting arbitrary raw I/O handle values (RawFd, RawHandle, or RawSocket) should be unsafe if they can lead to any I/O being performed on those handles through safe APIs.

If we shy away from these unsafe blocks, we’ll limit ourselves to the polling 2.x version indefinitely, which might not be in our best interest. The transition to acknowledging I/O safety aligns with the broader Rust community's efforts towards safer code.

I also propose taking additional steps to increase the code quality: enable lints like unsafe_op_in_unsafe_fn, which will be enabled by default in Rust's next edition, and the clippy::undocumented_unsafe_blocks/clippy::multiple_unsafe_ops_per_block combo to guarantee that all unsafe operations are thoroughly documented and reviewed.

[CBenoit]

Following my comment above, I suggest enabling these lints:

#![warn(unsafe_op_in_unsafe_fn)]
#![warn(clippy::undocumented_unsafe_blocks)]
#![warn(clippy::multiple_unsafe_ops_per_block)]

[keepsimple1]

We cannot remove #![forbid(unsafe_code)] just because one dependency lib changed its API to be unsafe. Sorry.

[keepsimple1]

If we shy away from these unsafe blocks, we’ll limit ourselves to the polling 2.x version indefinitely, which might not be in our best interest. The transition to acknowledging I/O safety aligns with the broader Rust community's efforts towards safer code.

I am all for upgrading to the latest version of any dependency. But this particular API change in the lib seems making things worse, not better:

It changed into unsafe with preconditions that are basically non-checkable (fd being valid). I am sad to see this change as I really liked the library. I really hope such change will not be a trend going forward in the name of I/O Safety.

Back to reality, yes looks like we have a problem with this dependency. As you said, we cannot limit to polling 2.x forever. I will try to see if polling would change back or have new safe API, or maybe some other replacement. My goal is to keep mdns-sd safe code only.

[CBenoit]

It changed into unsafe with preconditions that are basically non-checkable (fd being valid). I am sad to see this change as I really liked the library. I really hope such change will not be a trend going forward in the name of I/O Safety.

Could you please clarify what you mean by "non-checkable"? My understanding is that a file descriptor remains valid from the moment it is returned by the OS until close is called on it, and this can be manually checked, you probably did so when you wrote the code. However, the compiler cannot automatically verify this when using RawFd, in the same manner it cannot check raw pointers.

While I'm not here to advocate strictly for or against I/O safety, I do think it’s unlikely to "die" anytime soon, and we’ll have to consider it. I'd encourage looking into the RFC for a deeper dive into this topic and the ongoing "trend".

We cannot remove #![forbid(unsafe_code)] just because one dependency lib changed its API to be unsafe. Sorry.

I understand and respect your position, and of course it’s your call. I’m all for avoiding unsafe when it’s not absolutely necessary too. However, considering the situation, we are faced with only these alternatives:

• Choose not to upgrade, which preserves the current state but may limit future enhancements and compatibility. Downstream crates may also end up with duplicated dependencies. (In our case, this is the main motivation for opening this PR.)
• Avoid writing the code that requires unsafe blocks. The reality being that not all functionalities can be implemented using only safe Rust, which is why Rust offers the unsafe escape hatch. The RawFd API never was truly safe.
• Switch to another library which hides the unsafe code from you. For example mio is exposing a safe API. which is not taking RawFd.
• Of course, as you mention, negotiating with polling maintainers to add back a safe API is also an option. This must be an API which does not use RawFd however, I don’t think they will just drop the unsafe keyword and keep everything else as-is.

The first alternative is not good, and the second one almost sounds like a joke, so considering them as no-go we only have the last two options.

About mio, I’m not sure how good it plays with socket2. I haven't personally tested this integration, but I recall @irvingoujAtDevolution mentioning some challenges in this area.

If it’s possible to go for the last option, that’s great. I’ll be following this.

[keepsimple1]

Could you please clarify what you mean by "non-checkable"?

I meant that fd is a value provided by the OS, and it is very hard for the application to be sure it's valid all the time. I'm new to I/O safety, but I think this comment in a different repo explains a lot better than myself about the confusion I have.

And I submitted a draft PR in polling to understand the possibility of changes. Will follow up on that.

[keepsimple1]

To move forward, while I'm waiting on my PR in polling, you can upgrade socket2 version to latest first, if you want to. I'm okay if you create a new PR for socket2 update, or just use this one and create a new issue to track the polling update.

[CBenoit]

Sounds good to me. @irvingoujAtDevolution Do you think you can you look into this?

[irvingoujAtDevolution]

Sounds good to me. @irvingoujAtDevolution Do you think you can you look into this?

Will do, thanks for you all!