search

faern / oneshot

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

Fix and improve the Future impl on Sender #20

Closed faern closed 2 years ago

faern commented 2 years ago

Most importantly this fixes the Future implementation on Sender.

This PR removes the last usage of SeqCst ordering and replaces it with appropriate weaker orderings.

faern commented 2 years ago

Ping @Cassy343! It would be great if you would like to give this a review and help me verify that these changes are correct. I'll leave some inline comments in some places to clarify.

faern commented 2 years ago

Hey @Cassy343. I know you are busy. I just want to check if you think you would want to review this at some point? Or if I should look through it one more time myself and go ahead with the merge myself.

Cassy343 commented 2 years ago

Hey, I can review the other changes but did you see my comment on the changes to wait_for_unpark? Many of these changes seeme to concern that and those changes confused me. I'll look at the rest tonight.

faern commented 2 years ago

No I can't find any comments from you in this PR :open_mouth: Can you link to it?

Cassy343 commented 2 years ago

After a quick Google search it turns out I had fallen into this trap... I'll fix that after work today or if I have a spare moment before then

Cassy343 commented 2 years ago

These changes look good to me, glad to see this fixed up! Also good catch realizing that an acquire load of the UNPARKING state lets us do relaxed operations thereafter on the receiving side.

faern commented 2 years ago

Great! Are you happy with the PR as is? If so, I can merge later today.

I'll do some sanity checking that we have not made any breaking change in the API, and fix the changelog. Then I might release a patch release.

Cassy343 commented 2 years ago

Yeah I'd say I'm happy with it as is. I have to get to bed now so I can't sanity check it again, but if you feel comfortable I think its fine to merge later today.