search

keepsimple1 / mdns-sd

Rust library for mDNS based Service Discovery
Apache License 2.0
89 stars 38 forks source link

Bump polling and socket2 version to latest #172

Closed irvingoujAtDevolution closed 4 months ago

irvingoujAtDevolution commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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:

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

irvingoujAtDevolution commented 5 months ago

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

Will do, thanks for you all!