How about Inner being `Sync` even when `T` isn't?

[najamelan]

Both the futures library and the oneshot crate have:

unsafe impl<T: Send> Sync for Inner<T> {}

But async-oneshot has:

unsafe impl<T: Sync> Sync for Inner<T> {}

This is a quite awkward API, as awaiting a receiver in a future that will be spawned on a threadpool now requires T: Sync. Double check carefully, but it doesn't really make sense either. A oneshot should just deliver the T in one piece in one thread and never allow concurrent access to it.

[jjl]

That makes sense to me.

However, I've been experimenting locally and i think i can turn it into a capacity 1 channel (i.e. reusable) without losing performance (in fact it's faster because i'm doing refcounting myself instead of letting arc do it so the creation/destruction cycle is about halved in time!). I might change the crate name to better reflect this, but I will roll this change in.

[jjl]

On closer examination, adding Sync is not this straightforward. One of the things we do is assume there will be no more than two threads trying to operate on a single channel at a time. This gives us linearisability with acquire/release semantics.

Now, you could feasibly send a reference to two threads and since all our apis only require &self, you've violated our assumption and now you're racing.

...which suggests that as it stands, this library should never have had a Sync bound in the first place, regardless of T!

But people want Sync because their executors require it. So I think we're going to have to upgrade to a SeqCst barrier instead of Acquire/Release, which might have performance implications on weakly ordered platforms.

That would allow me to provide a Sync bound regardless of whether T is Sync.

Does that make sense to you?

[najamelan]

I see. If you don't want to allow more than one access to each end of the channel, what is the advantage of having a &self api? I think all other channel implementations require &mut self.

As to the atomic, maybe you can have the sync option as a feature, so people can turn it off with default-features false if they need the performance on weakly ordered platforms.

[jjl]

It makes it easier to work with some of the futures composition primitives, which can get a bit hairy if you need to keep mutable refs about.

You also correctly predicted what I did a few minutes ago in my local copy heh.

[jjl]

I've double checked and looks like I never introduced the change to require only &self to do a send/receive in any released copy (I've been working on a bit of a refactor for a while now, but i haven't done a release in ages).

So the only thing you could do is check if the channel is closed on the sender, so I think it should be fine with acquire/release ordering.

Which leaves us with an additional option:

• Upgrade to SeqCst and enable &self everywhere.
• Continue to require &mut self for mutations and don't upgrade to SeqCst

[jjl]

When I merge #18 and release 1.0.0-rc0, this change is included. Please provide feedback on these changes as there are quite a few.