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

poll_fn and Unpin: fix pinning #156

Closed

Conversation

FrankReh
Copy link
Collaborator

Keeping this crate's poll_fn in sync with that of rust's library/core/src/future/poll_fn.rs.

Their merge provides all the details.

https://github.com/rust-lang/rust/pull/102737

None of this crate's uses of poll_fn are affected. It protects against misuse in the future which could have led to UB as the above mentioned PR points out with an example of a double free becoming possible.

Keeping this crate's poll_fn in sync with that of rust's
library/core/src/future/poll_fn.rs.

Their merge provides all the details.

    rust-lang/rust#102737

None of this crate's uses of poll_fn are affected. It protects against
misuse in the future which could have led to UB as the above mentioned
PR points out with an example of a double free becoming possible.
@FrankReh FrankReh requested a review from Noah-Kennedy October 30, 2022 17:03
@FrankReh
Copy link
Collaborator Author

@Noah-Kennedy Please check that my changes look the ones introduced with that PR referenced because I did them by hand. I didn't pull the git commit and apply it to this file.

@Noah-Kennedy
Copy link
Contributor

Thinking about this now, do we even need our own poll_fn? This was added when poll_fn wasn't stable in the stdlib, and the tweaked version of this function will be stable in several days.

@FrankReh
Copy link
Collaborator Author

Does that mean tokio-uring MSRV becomes 1.64.0?

@Noah-Kennedy
Copy link
Contributor

We don't really have an MSRV policy for tokio-uring.

@FrankReh
Copy link
Collaborator Author

So far, this crate has been piggy backing off tokio. And tokio has an MSRV that goes back six months. We could merge this with a note saying it should be removed when 1.64.0 becomes the tokio MSRV. So we continue to track tokio.

@Noah-Kennedy
Copy link
Contributor

I'm not entirely sure which older Rust versions we work on currently. We may not even work on tokio's oldest supported version, and I don't see any reason that we should maintain an MSRV at this point.

@FrankReh
Copy link
Collaborator Author

By not maintaining an MSRV, you mean you think it's okay to require current be used by anyone wanting to use this crate? Then the README should probably say that.

But I could be in above my head here. You can make these changes without noise from me. You've been at this successfully a long time. No hard feelings if you let this PR wither and then get closed.

@ollie-etl
Copy link
Contributor

My view: this crate is marked as both experimental and likely to changed. We should not even be attempting to keep a MSRV in tokio or rustc.

@FrankReh
Copy link
Collaborator Author

I would just like there to be a better reason someone had to use the 1.64.0 compiler. For this, it seems easy to not change what we were doing.

@ollie-etl
Copy link
Contributor

It could always be punted to an issue

@FrankReh
Copy link
Collaborator Author

FrankReh commented Dec 3, 2022

@Noah-Kennedy what should we do with this? I think it safer to just merge the changes and remove the code at a later time. But the other way is okay too.

@Noah-Kennedy
Copy link
Contributor

Let's close this and just get rid of our version of this method.

@FrankReh
Copy link
Collaborator Author

FrankReh commented Dec 3, 2022

Obsoleted given merge of #194

@FrankReh FrankReh closed this Dec 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants