Add ?Send futures support in async-await branch

[nWacky]

Is your feature request related to a problem? Please describe.
Sometimes it's necessary to run not Send future in juniper handler (for example to get data from database). However, when trying to run such future I get the following error:

error[E0277]: `dyn core::future::future::Future<Output = std::result::Result<User, Error>>` cannot be sent between threads safely
  --> mod.rs:15:1
   |
15 | #[juniper::object(name = "Mutations", Context = Context)]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `dyn core::future::future::Future<Output = std::result::Result<User, Error>>` cannot be sent between threads safely    

Describe the solution you'd like
It would be nice to be able to use an option to use !Send futures in juniper, ideally with ?Send option on #[juniper::object] macro (like in async_trait )

I have tried removing Send bound from everywhere and it works fine, but then it's not possible to use futures from juniper as Send futures (for example, I cannot call req.execute_async(schema, context).boxed().await)

I don't think that it would be possible to have such option on #[juniper::object] macro because Send marker is defined in trait, not in it's implementantion.
I think it could be done with a cargo.toml feature, but then it won't be possible to use both Send and !Send futures in one project

Describe alternatives you've considered
I have considered changing database connection function, but it decreases performance

[theduke]

I don't think we'll be able to fit !Send futures into the design, even with a feature. Things are already a bit messy with GraphQLTypeAsync layered on top of GraphQLType (this might change a a bit in the future).

Supporting !Send futures would require all resolved Futures to be without the Send bound and would be restricted to one thread.
I don't really see a lot of use cases where this would be sensible.

Your use case seems to be dealing with a synchronous database connection?
You can't (well, shouldn't) use those anyway in a async environment, since you are not supposed to block a executor thread for longer periods.

Longer synchronous operations need to be delegated to a thread pool. Which is totally sensible for database queries if your DB api does not support async (yet).

Below is a super simple (suboptimal) code snippet as an example for such a threadpool that I wipped up last week.
I'm sure a good crate with a better implementation will emerge pretty soon.

#[derive(Clone)]
pub struct SyncThreadPool {
    pool: Arc<Mutex<threadpool::ThreadPool>>,
}

impl SyncThreadPool {
    pub fn new(threads: usize) -> Self {
        let pool = threadpool::ThreadPool::new(threads);
        Self {
            pool: Arc::new(Mutex::new(pool)),
        }
    }

    pub async fn execute<T, F>(&self, f: F) -> Result<T, failure::Error>
    where
        T: Send + 'static,
        F: FnOnce() -> T + Send + 'static,
    {
        let (tx, rx) = futures::channel::oneshot::channel();
        let wrapped = move || {
            let output = f();
            tx.send(output)
                .map_err(|_| ())
                .expect("Could not send completion message");
        };
        self.pool.lock().unwrap().execute(wrapped);
        rx.await
            .map_err(|_| failure::format_err!("Pool execution failed"))
    }
}

[tyranron]

Your use case seems to be dealing with a synchronous database connection?
You can't use those anyway in a async environment, since you are not supposed to block a executor thread.

Nope, it's async, however is !Send due to use Rc under-the-hood. It's used in pair with actix-web which runs everything on single thread executor, so plays very nice for us so far. Changing it to Arc feels just like redudant performant cost.

Supporting !Send futures would require all resolved Futures to be without the Send bound and would be restricted to one thread.
I don't really see a lot of use cases where this would be sensible.

Usual CRUD with database is quite a case. There is no need to share futures between threads. Each request does it job in its own thread quite OK as there is no CPU-bound tasks.

---

As for me the best solution would be to have juniper being Send-agnostic. It should not restrict to use Send in its signatures, while the overall request.exec_async() result is Send if all the downstream futures are Send. This will allow the full flexibility for library user to choose the desired guarantees.
However, as I see, it's barely possible 😕
I haven't dig into juniper design deep yet. Would you be so kind to point us to exact pain points why ?Send cannot be baked into juniper and maybe we will invent something for this? At least we will try 🙃

[theduke]

As for me the best solution would be to have juniper being Send-agnostic

I agree, in theory.

In practice I think it's impossible right now.

Would you be so kind to point us to exact pain points why ?Send cannot be baked into juniper and maybe we will invent something for this?

The problem is that traits are involved. The resolver methods on the GraphQLAsync type currently return a boxed future. If the Rust gets support for -> impl Trait return types in trait methods, we could switch to that.
But in both cases we need a Send bound to tell the compiler that the involved futures are so. Otherwise the compiler can't know that they in fact are, which results in juniper not working for multi-threaded runtimes.

I think this would theoretically be possible if each GraphQLType specified the future used as an associated type (it could be a wrapper future type with From impls) and then the async resolver returns the associated type. That could allow the compiler to deduce that the whole resolver tree is Send without bounds.

But this is currently impossible because a lifetime is involved, which would require higher kinded types/ATC support.

I'll think about this more a bit (and would very much welcome a solution), but I'm pessimistic.

(yes in theory we could have a feature that toggles on/off the sync + send bounds, but that would be really messy)

It's used in pair with actix-web which runs everything on single thread executor

I thought that just applies to actix (aka the actor framework), not actix-web?

Are using actors together with actix-web?

Usual CRUD with database is quite a case. There is no need to share futures between threads. Each request does it job in its own thread quite OK as there is no CPU-bound tasks.

The benefit of async for juniper is allowing multiple DB/http/whatever requests to run concurrently.
Without that, you can just use juniper in a old school synchronous way.
The result would be the same, since the majority of time will be spent waiting on the db, and everything would happen one thing after the other. So involving futures brings no benefit.

The only benefit would be allowing multiple requests to make progress on the same thread, but I don't think that makes sense any sense when sync network calls are in the mix. Everything would block/starve each other and your server would be very limited in throughput. You'd need multiple threads anyway.

An alternative for your situation could be running a dedicated runtime for juniper and integrate with the actix runtime via channels.

[tyranron]

The only benefit would be allowing multiple requests to make progress on the same thread

That's exactly the point.

but I don't think that makes sense any sense when sync network calls are in the mix.

There is no sync/blocking calls in our case.

I thought that just applies to actix (aka the actor framework), not actix-web?

Are using actors together with actix-web?

We're using Actors just for WS connection, for anything other actix-web is enough. While actix-web uses multiple threads to execute requests, it does not require used Futures being Send. When request arrives it goes to one of spawned worker-threads where single-threaded executor executes it. So request processing can be totally Send-free and do not pay unnecessary synchronization costs. Such actix-web design has its own downsides like impossibility to use tokio-fs (as it requires thread pool executor), and so has its advantages too, especially for IO-bound tasks (TechEmpower benchmarks are illustrative about that). Of course if CPU-bound tasks are involved a thread pool executor with work stealing should be a better choice, but for trivial CRUD to database it's a rare case to have CPU-bound tasks.

An alternative for your situation could be running a dedicated runtime for juniper and integrate with the actix runtime via channels.

Yup, that sounds like a good hack for an immediate solution. Thanks for pointing to it 🙏

I'll think about this more a bit (and would very much welcome a solution), but I'm pessimistic.

Thank you for detailed explanation, pointing to the problem causes and paying so much attention to all this. In next few days I'll try to dig into code deeper and think about it from my side too.

[theduke]

There is no sync/blocking calls in our case.

Ah I misunderstood the problem, yes of course it makes sense in this context.

What I don't understand is why the Send bound is causing a problem for you though! Executing a : Send future on a single threaded runtime should be perfectly possible.

Can you share a bit more of your code?

The error message indicates that it is actually your resolver that relies on something that is !Send. Either the Context or some code in the resolver.

[tyranron]

The error message indicates that it is actually your resolver that relies on something that is !Send.

Yes. Our DB interaction layer provides !Send interface as uses a lot of Rc-based interior mutability. This layer is used in #[juniper::object] fields, so they are !Send.

So, the overall flow for request is the following:
actix-web handler (can be ?Send) -> juniper schema execution (requires Send) -> juniper::object fields resolution (uses !Send DB layer).

If we have had Send DB layer, there would be no problem, as you've referred above Send futures are perfectly fine in ?Send contexts. However, our downstream futures are !Send, so juniper just does not allow them to be used in schema as strictly requires to return Send futures to upstream.

[theduke]

I'm closing this for now. The problem is on my radar.

Feel free to continue discussion in the main async issue ( #2 ).

[theduke]

@nWacky good news, I think I might have found a way to do this reasonably well.

[tyranron]

This is still a big pain point in the current juniper design. Integrating tightly with juniper to power it with clever Context ends up leaking Send pretty everywhere, making us to annotate every extension with SendWrapper, FutureSendWrapper and other beasts just to naively satisfy unnecessary Send requirement.

I'd like to elaborate on this, however don't really understand the right direction.

good news, I think I might have found a way to do this reasonably well.

@theduke would you be so kind to share some thoughts about how this can be acomplished?

@davidpdrsn @LegNeato do you have any ideas/thoughts about that?

[LegNeato]

I haven't looked into this at all but I wonder if we can use something like https://rust-lang.github.io/async-book/07_workarounds/04_send_approximation.html