What shall Sync mean across an .await?

Yes, that is a great idea!

Another thought is whether the automatic derivation of Sync could be enlarged to encompass types that are Send and where nothing can be done with a shared reference to that type. Would this break assumptions regarding what is allowed in unsafe code?

It might even be possible to derive at the use-site that if I have an “impl Stream<Item = String> + Send” (or an exclusive reference thereof) in my data structure, then neither can I nor anybody else make use of possibly existing other methods, so by the argument above the data structure could be Sync (if nothing else within it prevents this).

Is this reasoning correct?


This all goes in the direction of making the compiler more clever, which in turn trains people to expect that things in Futures only need to be Send, not Sync. To my mind this should only be done if we can also ensure that compiler errors in case of hitting a limit in the cleverness are reported close to their true cause. Then the developer will quickly see that they are running into a compiler limitation and work around it (instead of wasting hours to reconcile their mental correctness proof with the imperfect world).

Automatic derivation has a downside when you consider breaking changes. If your type is Sync and a change makes it not Sync anymore, that’s a breaking change that’s possibly overlooked. With the current rules at least the rule is very easy and the same for any auto trait. Adding special extra rules for Sync makes this a lot more brittle.

Furthermore every struct does offer something that can be done with a shared reference: Access fields. Even if they’re private, it is possible inside the implementation of the struct. It would then need to be the case that from the point of view of the implementor of a type’s API, that type is not Sync, whereas looking from outside it is. Quite a novel concept that probably has lots of problems attached to it.

The first problem here is that Stream does have a by-reference method

fn size_hint(&self) -> (usize, Option<usize>)

But apart from that, impl Stream usually means that the thing actually has a concrete type that’s just not mentioned. If you’re not careful about what you allow, a shared reference to it could actually be shared with some different code that knows the actual type and can do something with it. I’m not saying it’s impossible to argue like this, maybe the compiler could deduce something of the like in a sound manner. But even then the stability problem kicks in again. Best demonstrated by Stream itself, which didn’t have the size_hint method in the past. Adding that method would, in the presence of such new rules, have been a breaking change, and IMO quite surprisingly so.

I would also believe, regarding the argument of nothing can be done with a shared reference of a type, it can be Sync, that the wrapper type I proposed above does allow everything you could need. Using it furthermore would demonstrate some commitment to the fact that your type is indeed supposed to be Sync and will stay Sync (so no surprising breaking changes by accident). If you only apply the wrapper to non-Sync fields, you could even implement Stream with a sensible size_hint; the implementation of that method just can’t access some of the fields.

1 Like

Yes, indeed — thanks for this acute demonstration (I hadn’t checked the Stream trait in detail for a while and thus didn’t have the size_hint in mind). So automatic derivation should stay as clever as it is now.

Wow, I didn't grasp it when you first posted it but SyncWrapper is a pretty impressive trick, to the same level as Cell or Pin, even. If that really is sound (I wonder about pointer provenance and stacked borrows, but haven't thought too hard about them) then it seems like something that could go in the standard library.

SyncWrapper works by the same principles as Cell, disallow all bad references to the underlying data. In the case of Cell, this is all references that may allow reentrant accesses to the underlying value. In the caee of SyncWrapper, this is all shared references to the underlying value that come from a shared reference to SyncWrapper.

You can even draw parallels to Mutex. Mutexes only allow references to the underlying value while you have exclusive access to the value. It enforces this via runtime checks and blocks if need be. SyncWrapper only allow references to the underlying value while you have exclusive access to the value. It enforces this via compile time checks.

Thus the same Send/Sync bounds that apply to Mutex also apply to SyncWrapper because SyncWrapper is a mutex, just a compile time checked mutex.

1 Like

Yes, I’m using SyncWrapper as well (though I call it Unshared) and have had multiple people on the #black-magic discord channel agree it’s sound. I don’t think there’s any way it could violate pointer provenance/stacked borrows rules given that the actual reference manipulation is all safe code.

The biggest downside is it blocking any really useful implementation of Debug or Clone or other shared reference taking traits.

5 Likes

Worth noting this is also how pin works: it disallows getting mutable references to the underlying data (in safe code).

2 Likes

Can someone ELI5 the bounds of the problem here?

I get the general concepts (we wants to be able to use non-Sync types in async functions, but futures are sometimes passed to multi-threaded executors), but I don't really understand what the proposed solutions are and how they would work?

FWIW, I agree that this is sound. I don't think there are any aliasing issues here. Stacked Borrows does not care about Send/Sync, and other than that this type is entirely safe. Note that one crucial aspect to soundness is that SyncWrapper is not Copy (even disregarding the Clone issue).

(In formal RustBelt terminology, SyncWrapper can have a trivial sharing invariant, which then in turn implies that it is trivially Sync.)

3 Likes

A proposal for such a lint rule is now available: https://github.com/rust-lang/rust-clippy/pull/5423

3 Likes

Fascinating, I believe &Unshared<T> corresponds to the tag capability from Pony, in that the only useful thing you can do with it is compare its address.

3 Likes

Shouldn't it also check that futures are Sync? While Sync seems meaningless for Future itself since poll takes &mut self, the problem is that as an auto trait, the !Sync propagates to anything that owns the Future, and these objects are likely to end up used in an async-await block.

To provide a concrete example and tie things back to the hyper::Body::wrap_stream function mentioned in the original post, I've run into this issue trying to use rusoto_s3 with Hyper. The future type returned by the rusoto_s3::S3::get_object method is Pin<Box<dyn Future + Send>> , without Sync. When combining these futures into a stream, the resulting Stream therefore does not impl Sync . However, hyper::Body::wrap_stream has a Sync bound on its argument because the stream it is passed ends up wrapped inside a Request struct that needs to be Sync so that it can be borrowed across an await .

rusoto_s3 uses the async_trait library to define the S3 trait, so I've opened an issue there about making those futures Sync.

To my mind the proper fix (which will probably be my next weekend project) is to remove the Sync requirement from wrap_stream by employing the SyncWrapper inside Hyper’s Body. I consider it highly problematic to require things to be Sync which don’t need to be, because it communicates an incorrect expectation: the stream (in the example) is not actually shared among multiple threads, the Sync requirement is incidental. Let’s not pollute the whole ecosystem.

8 Likes

+1 on this. This seems to be mostly a Hyper problem, which is caused by a compiler limitation - and not a general Sync issue that needs to be solved.

Futures having to be Send makes sense for being able to use them with executors which move them between threads in order to make use of work-stealing strategies. That makes sense, but also can be somewhat inconvenient (and will cause some unnecessary overhead if those Futures are used in a pure singlethreaded context).

But Sync is a different story. There is really no good reason why a Future should be Sync. If there is a compiler limitation it is better to work around this in the library than to spread the problem everywhere.

1 Like

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.