faern / oneshot

Oneshot Rust channel working both in and between sync and async environments
Apache License 2.0
80 stars 9 forks source link

Adding into_raw() and from_raw() methods #29

Closed bendk closed 1 year ago

bendk commented 1 year ago

See issue #28.

bendk commented 1 year ago

Created a PR to continue the discussion from #28. What do you think? I considered adding tests, but I couldn't think of a meaningful way to do it.

bendk commented 1 year ago

Updated the docs and added tests. It's a good thing you suggested that, because I forget to call std::mem::forget in the initial code.

bendk commented 1 year ago

Thanks for working through this one with me. I moved the test into the tests directory and the docstrings are looking great to me. Did I miss anything?

For what it's worth: I don't think we're actually going to need this, but I still think the functionality could be useful to someone else (and maybe us in the future).

faern commented 1 year ago

Oh yeah. I won't merge right now. The CI don't pass. You will have to look into that.

bendk commented 1 year ago

I think the CI issues should be fixed.

The pointer type question is a hard one. *mut () doesn't seem perfect, but Rust does have a convention of using () to represent untyped pointers (https://doc.rust-lang.org/std/task/struct.RawWaker.html). I think I prefer that over c_void.

What about making the Channel struct public, but none of its fields or methods? That would allow returning * Channel<T>, which actually is the correct pointer type.

In the end though, I think you're right that users are going to end up casting it to so maybe it doesn't matter too much.

faern commented 1 year ago

but Rust does have a convention of using () to represent untyped pointers

Awesome. Examples of this being used by std was just what I needed to be convinced. Let's keep it at *mut ()

I think the CI issues should be fixed.

Sadly not. It's still missing on some combinations of features. You either need to use try_recv in the tests or cfg out the tests when certain features are off. I'd vote for the former. None of your tests should need to block.

bendk commented 1 year ago

try_recv works for me, I just updated the tests to do that. Tell me if there's a better way to handle this CI feedback loop.

faern commented 1 year ago

Not sure why the waker size tests fail. I will try to look into that in the weekend.

bendk commented 1 year ago

Not sure why the waker size tests fail. I will try to look into that in the weekend.

Those fail for me as well when I run cargo test locally. My guess was that newer versions of Rust are optimizing the struct better so that the padding for one field is used for part of another field or something like that.

faern commented 1 year ago

Apparently the size of the ReceiverWaker went from 24 to 16 bytes in Rust 1.65. I can't find anything in the release notes that explains why. The size of std::task::Waker is 16 bytes in both older and newer Rust. But I guess some internal change made it aware of invalid representations, that it could then optimize with the help of, to fit the ReceiverWaker enum discriminant into that space.

I have now pushed fixes to this to main. Can you please rebase and push your code again?

bendk commented 1 year ago

Sure. The tests are now passing locally for me as well.

faern commented 1 year ago

Thank you for the contribution! Fixing the broken CI took a few days since I temporarily did not have access to my GPG key, and I wanted to sign the commits. But now it's fixed, and your PR is merged. Hopefully this is helpful to someone :)

bendk commented 1 year ago

Awesome, thanks for working with me on this one.