-
Notifications
You must be signed in to change notification settings - Fork 5
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
Waker update synchronization #20
Comments
looks like you're right. i have a PR on this repo open for a rewrite that makes it usable more than once. i don't think that version suffers from the same problem, but not many people have reviewed it. i'm also struggling for enthusiasm to write rust recently. i spent several months failing to get a rust job, so motivation to work on complicated low level things is not high right now :/ |
Sorry to hear about that, indeed rust jobs are not plentiful yet sadly... I decided to bite the bullet and write my own reusable one-shot channel. Guaranteeing truly lock-free send, receive, and recycle operations while totally preventing reallocations of the waker turned out to be much harder than for a plain one-shot channel, though. I think I have found an efficient way to achieve that, but I need to run it through Loom before I am totally confident it works. I will probably have a look later at the PR you mentioned. |
The PR turns it into a capacity one reusable channel. it pays some price for this relative to the platonic ideal of a oneshot channel, but i think there may be ways to overcome at least some of this, it just requires motivation. it's been a while since i looked at the actual oneshot code until yesterday, all my focus has been on that PR. strongly recommend you look at that. i am aware it oversynchronises in places, but i don't think it's far off the platonic ideal. my suspicion is that if you want the thing that makes this library interesting, waiting for receiver, you might not be able to get everything totally lockfree. there is also oneshot you may want to take a look at, although it would be nice if you could bring your updates over to this repo so that anyone using it can have an easy upgrade path. happy to hand out permission bits. |
TBH I actually don't need the wait-for-receiver feature, my initial review of this crate was because I was curious if it could be turned into a channel which receiver can recycle senders. The PR does appear to offer just that with the As you see my needs are pretty specific so I am afraid what I am implementing would not be of general interest and would further add complexity to what is already a fairly meaty PR (my implementation uses a redundant waker slot to achieve lock-freedom + a fairly non-trivial state machine). So I would suggest to close this issue since the PR seems to address the particular point I raised, though perhaps it would be good to put in the meantime a warning in the README until the PR lands. |
oh, that's an interesting approach. and quite an interesting problem, heh. |
Would the issue be addressed in oneshot/non-hatch-branch with the below change? It just zeros the send/recv bits during the move. Assuming that does what I think it does, it's also an extra atomic operation to prevent against an impossible situation in mainline, but it might still be useful. I'm also not seeing any changes in a quick little diff --git a/src/inner.rs b/src/inner.rs
index 262189f..c24bc47 100644
--- a/src/inner.rs
+++ b/src/inner.rs
@@ -47,6 +47,7 @@ impl<T> Inner<T> {
#[inline(always)]
pub(crate) fn set_recv(&self, waker: Waker) -> State {
let recv = self.recv.get();
+ self.state.fetch_and(!RECV, AcqRel);
unsafe { (*recv).as_mut_ptr().write(waker) } // !
State(self.state.fetch_or(RECV, AcqRel))
}
@@ -63,6 +64,7 @@ impl<T> Inner<T> {
#[inline(always)]
pub(crate) fn set_send(&self, waker: Waker) -> State {
let send = self.send.get();
+ self.state.fetch_and(!SEND, AcqRel);
unsafe { (*send).as_mut_ptr().write(waker) } // !
State(self.state.fetch_or(SEND, AcqRel))
} |
Sorry, couldn't find that branch, do you have a link? |
So only based on on the Lines 59 to 60 in 9d2f61d
Say:
Or is this scenario avoided in the |
Oh, the non-hatch branch is just mainline. IThe PR with the rewrite @jjl referred to was apparently called async-hatch, I can see how I was confusing, sorry :) Good catch on that fragment, I'll keep looking at this. |
correctly solving it would require introducing a LOCK flag which is taken before touching the wakers. i'm sure i've written it with such a flag before, but maybe i'm just remembering async-hatch... i had a look at the code again yesterday while procrastinating doing my paperwork and i here are the flags i've got in my current working copy (which alas is very broken because at some point i started refactoring it apparently...)
|
Hi,
I could not fully figure out how waker updates are synchronized.
If a waker is already registered (be it
send
orrecv
) and is later updated, theSEND
(orRECV
) bit stay set during the update so it would seem that nothing prevents a waker from being read while updated? Sorry if I missed something here.The text was updated successfully, but these errors were encountered: