Feature: Disabling async codegen

[parsadotsh]

I want to use Juniper together with SQLite, and there's only one connection to the database at a time. If I set the Context struct as such:

pub struct Context {
    ///this could be a database connection
    db: Arc<Connection>
}

Then trying to use

struct Query;
#[graphql_object(context = Context, noasync)]
impl Query {
...

gives an error, because the context isn't sync

noasync doesn't appear to do anything. I don't want to use GraphQLValueAsync, I want to use Juniper purely synchronously. What can I do?

[parsadotsh]

Commenting out this line:

juniper/juniper_codegen/src/util/mod.rs

Line 1116 in 39d1e43

#resolve_field_async

And then cargo clean and building again seems to have solved the issue. I think #resolve_field_async should be respecting the noasync attribute but it's not. Should be an easy fix?

[LegNeato]

Are any of your Query fields async? We hoist if so

[LegNeato]

Related: #840

[parsadotsh]

I didn't modify the actix_web example much, I don't see anything async here:

...
pub struct Context {
    ///this could be a database connection
    users: HashMap<i32, User>,
    db: Rc<Connection>
}
...
struct Query;
#[graphql_object(noasync, context = Context)]
impl Query {
    fn apiVersion() -> String {
        "1.0".to_string()
    }
    #[graphql(arguments(id(description = "id of the user")))]
    fn user(database: &Context, id: i32) -> Option<&User> {
        println!("{}", database.db.is_autocommit());
        database.get_user(&id)
    }
}
...

Looking through the proc_macro code, I think perhaps there's some work missing, there's a lot of comments like this:

juniper/juniper_codegen/src/util/mod.rs

Lines 722 to 723 in 39d1e43

	// FIXME: make this redundant.
	pub no_async: bool,

juniper/juniper_codegen/src/util/mod.rs

Lines 395 to 397 in 39d1e43

	// FIXME: make this unneccessary.
	"noasync" => {
	output.no_async = Some(SpanContainer::new(ident.span(), None, ()));

As I mentioned above, I removed that one line #resolve_field_async from the proc_macro and everything (and all the fields) seems to be working with execute_sync now.

#840 is discussing more about supporting !Sync Contexts for async resolvers and normal async execute.

[parsadotsh]

into_enum_tokens and into_input_object_tokens both have:

if !self.no_async {
      body.extend(_async)
}

in the end. into_tokens doesn't. I did the same there (changed output to mut, removed #resolve_field_async and added the above block but for resolve_field_async) and I think that fixes it.

I was making a PR but one of the tests ran into an error. In this test:

juniper/juniper/src/macros/tests/impl_object.rs

Lines 36 to 42 in 39d1e43

	#[crate::graphql_object(
	scalar = crate::DefaultScalarValue,
	name = "Query",
	context = Context,
	// FIXME: make async work
	noasync
	)]

Query has noasync but is executed at the end using async execute which causes an error. I didn't touch the test just in case I misunderstood something.

[LegNeato]

Nice catch! I think that test is just a copy and paste error.

[tyranron]

@lucashyper noasync is nothing to do with your issue, it's just a some performance-motivated rudiment from sync -> async migration times.

You won't be able to use !Sync types in juniper::execute. And you cannot use juniper::execute_sync if your schema has at least one async resolver (which is unavaoidable, I guess, giving that you're using juniper_actix). And juniper_actix has no way to use juniper::execute_sync, as it has no sense for it.

The problem with juniper::execute is that Rust has no stable GATs yet. So, to use async/.await in traits, we need to use something like #[async_trait] which involves boxing and trait objects. The later, however, are not transparent over auto-traits, and that's exactly what is causes the issue. So Send/Sync is a hard requirement in juniper, and cannot be avoided at this point.

But, there is a trick for actix-web: just use send_wrapper. As your futures won't migrate between threads beacuse of how actix-web runtime works, the [SendWrapper] will never ever panic, and you may use any !Send/!Sync types inside it without any additional synchronization. We're using juniper this way in our production code base almost a year, and never had any troubles with it.

I hope, I've explained well the root causes of this issue, and the way to resolve it, so I'm closing the issue now, as there is nothing juniper can do from its side at the moment. Feel free to reopen, if anything else is needed.

[parsadotsh]

@tyranron Thank you for the explanation for !Sync in juniper::execute. However I think I'm missing something here:

1- I'm only using actix as a developing tool for now, later it'll be without any web framework, purely synchronously. I'm not really using juniper_actix, only the graphiql and playground parts directly. For the graphql endpoint I have this: (I just edited the code from juniper_actix to use web::block)

...
let (is_ok, gql_response) = actix_web::web::block(move || {
    let conn = Connection::open_in_memory().unwrap();

    conn.execute_batch(INIT).unwrap();

    let context = Context::new(Rc::new(conn));

    let gql_batch_response = req.execute_sync(&schema, &context);
    let gql_response = serde_json::to_string(&gql_batch_response);
    (gql_batch_response.is_ok(), gql_response)
})
.await?;
...

2- None of my resolvers are async

[parsadotsh]

@tyranron The problem can be reproduced with just this, the following doesn't compile:

use rusqlite::{Connection};
use juniper::{GraphQLObject, graphql_object};

pub struct TestContext {
    conn: Connection,
}

impl juniper::Context for TestContext {}

pub struct User;
#[graphql_object(context = TestContext)]
impl User {
    fn id(&self) -> i32 { 
        42
    }
}


fn main() {
    println!("Hello world!");
}

Gives error:

`RefCell<rusqlite::inner_connection::InnerConnection>` cannot be shared between threads safely
within `TestContext`, the trait `Sync` is not implemented for `RefCell<rusqlite::inner_connection::InnerConnection>`rustcE0277
main.rs(1, 1): required by a bound in this
async_await.rs(19, 20): required by this bound in `GraphQLValueAsync`
main.rs(1, 1): required because it appears within the type `Connection`
main.rs(4, 12): required because it appears within the type `TestContext`
`RefCell<hashlink::lru_cache::LruCache<Arc<str>, rusqlite::raw_statement::RawStatement>>` cannot be shared between threads safely
within `TestContext`, the trait `Sync` is not implemented for `RefCell<hashlink::lru_cache::LruCache<Arc<str>, rusqlite::raw_statement::RawStatement>>`rustcE0277

[parsadotsh]

I'm on juniper = "0.15.7"

Looking at the proc_macro code I don't see any mechanism to recognize whether the resolvers are async or not. I suspect the transition from manual noasync to automatically recognizing async vs sync was never completed.

[tyranron]

@lucashyper

from manual noasync to automatically recognizing async vs sync was never completed.

It's not like that. It's more like juniper_codegen always generates code both for sync and async resolving. The actual code is invoked whether juniper::execute or juniper::execute_sync is called. But it always generates both implementations. And while async implementation requires Sync - you have your error, despite the fact you don't use async resolving.

noasync attribute's argument doesn't mean "do not generate code for async resolution", no. It means "in context of async resolution, use sync code asap to resolve me". It's only a performance hint, nothing more.

[parsadotsh]

@tyranron I see. So reading your first comment again, if I understand correctly you're saying that: If I'm not using async anywhere, and none of my of resolvers are async, and I only want to use execute_sync, I should use send_wrapper to wrap the !Sync Context as a workaround?

noasync attribute's argument doesn't mean "do not generate code for async resolution"

Can this (manually disabling codegen for async) be a feature request then? Changing those couple of lines (#973 (comment)) was enough for noasync to act as such, and I'm using my patched version without any problems now, even though as you said noasync wasn't meant for this originally. Or does this conflict with future plans for the library?