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

EmptyMutation breaks Sync trait #439

Closed
j-maas opened this issue Oct 16, 2019 · 3 comments · Fixed by #443
Closed

EmptyMutation breaks Sync trait #439

j-maas opened this issue Oct 16, 2019 · 3 comments · Fixed by #443
Labels
bug Something isn't working

Comments

@j-maas
Copy link
Contributor

j-maas commented Oct 16, 2019

Since Rocket requires the data it manages to be thread-safe, I encountered an issue where the Juniper Schema containing a thread-safe database pool would not be recognised as implementing Sync. I've initially opened an issue at the Rocket repo.

The gist is that I would get blocked by a compile error:

error[E0277]: `std::cell::Cell<i32>` cannot be shared between threads safely
  --> src/api.rs:27:1
   |
27 | / fn post_graphql_handler(
28 | |     context: Database,
29 | |     request: juniper_rocket::GraphQLRequest,
30 | |     schema: State<Schema>,
31 | | ) -> juniper_rocket::GraphQLResponse {
32 | |     request.execute(&schema, &context)
33 | | }
   | |_^ `std::cell::Cell<i32>` cannot be shared between threads safely
   |
   = help: within `juniper::schema::model::RootNode<'static, store::Query, juniper::types::scalars::EmptyMutation<store::Context>>`, the trait `std::marker::Sync` is not implemented for `std::cell::Cell<i32>`
   = note: required because it appears within the type `diesel::connection::AnsiTransactionManager`
   = note: required because it appears within the type `diesel::SqliteConnection`
   = note: required because it appears within the type `rocket_contrib::databases::r2d2::Conn<diesel::SqliteConnection>`
   = note: required because it appears within the type `std::option::Option<rocket_contrib::databases::r2d2::Conn<diesel::SqliteConnection>>`
   = note: required because it appears within the type `diesel::r2d2::PooledConnection<diesel::r2d2::ConnectionManager<diesel::SqliteConnection>>`
   = note: required because it appears within the type `store::db::Database`
   = note: required because it appears within the type `store::Context`
   = note: required because it appears within the type `std::marker::PhantomData<store::Context>`
   = note: required because it appears within the type `juniper::types::scalars::EmptyMutation<store::Context>`
   = note: required because it appears within the type `juniper::schema::model::RootNode<'static, store::Query, juniper::types::scalars::EmptyMutation<store::Context>>`
   = note: required by `rocket::State`

This is surprising, because the store::db::Database type actually implements Sync. I found that I could resolve the issue by not using juniper::EmptyMutation but instead doing

pub struct Mutation;

#[juniper::object(Context=Store)]
impl Mutation {}

Therefore, I assume that there is a bug in the implementation of EmptyMutation that prevents correct inference of the Sync trait.

While it is not a self-contained example, the error is present on this commit and is fixed in this commit.

@j-maas j-maas added bug Something isn't working needs-triage labels Oct 16, 2019
@LegNeato
Copy link
Member

I think we can just do:

unsafe impl<T> Send for EmptyMutation<T> {}

Because EmptyMutation never references T...it only uses it for PhantomData. That being said, I am unsure as I have never used unsafe in Rust before. @theduke ?

@j-maas
Copy link
Contributor Author

j-maas commented Oct 24, 2019

Wow, you guys are awesome! Thanks a lot!

@LegNeato
Copy link
Member

I pushed a point release with this fix in it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants