Skip to content
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

Why does Body need to be Sync? #2159

Closed
rkuhn opened this issue Mar 24, 2020 · 5 comments
Closed

Why does Body need to be Sync? #2159

rkuhn opened this issue Mar 24, 2020 · 5 comments

Comments

@rkuhn
Copy link

@rkuhn rkuhn commented Mar 24, 2020

In https://github.com/hyperium/hyper/pull/2048/files#diff-8fd688c7233290bc8ae01519ef707f1eR58 the Body was changed to require a Sync bound. This trickles down into Body::wrap_stream, which now requires the Stream to be Sync as well. A Stream struct basically is the description of a task out of which elements can be pulled. Requiring this task to be Sync amounts to saying that the poll_next function needs to be reentrant, but that runs counter to its signature which promises a &mut self reference (i.e. an exclusive reference). Somewhere in here something is broken, could you enlighten me exactly where the breakage lies?

@sfackler

This comment has been minimized.

Copy link
Contributor

@sfackler sfackler commented Mar 24, 2020

That is not the commit that made the change, this is: 4441372.

Reentrancy is not discussed in the definition of Sync that I'm aware of. The standard definition has to do with safe concurrent access to a shared reference to a type.

@seanmonstar

This comment has been minimized.

Copy link
Member

@seanmonstar seanmonstar commented Mar 24, 2020

The reason is described in the comment, pointing at rust-lang/rust#57017.

Without the requirement, this code fails to compile in a very confusing way:

match res.status() {
    StatusCode::OK => {
        let _chunk = res.body_mut().data().await;
    },
    _ => (),
}
@rkuhn

This comment has been minimized.

Copy link
Author

@rkuhn rkuhn commented Mar 24, 2020

@sfackler Thanks for the more precise link! Being able to safely share a reference between multiple threads includes the possibility of those threads concurrently making use of the reference, which in this case implies reentrancy of the one method offered by said reference.

@seanmonstar Good to know, so my conclusion is that the design of async/await forces Sync in unexpected places. Considering that Sync is “Send for references”, it becomes obvious that the async/await syntax is using these marker types in a way that overapproximates the problem: while it is true that these objects may be sent to other threads, the usual implication of Sync is concurrent access, which is lexically impossible in an async function body. As long as the async scheduler takes care to insert the appropriate optimizer and CPU barriers, an async function body should be regarded as a single-threaded piece of code, so both Send and Sync are not exactly the right abstractions to model what is going on. Do you think it makes sense to discuss this in a different place or forum closer to Rust core? I am rather new in this ecosystem and still learning its ways.

@seanmonstar

This comment has been minimized.

Copy link
Member

@seanmonstar seanmonstar commented Mar 24, 2020

You could bring it up in the issue I referenced, or if it only loosely matches, try internals.rust-lang.org.

@rkuhn

This comment has been minimized.

Copy link
Author

@rkuhn rkuhn commented Mar 25, 2020

done at internals, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.