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

render_to_string_await_suspense does not implement Send or Sync #540

Open
lizclipse opened this issue Dec 8, 2022 · 3 comments
Open

render_to_string_await_suspense does not implement Send or Sync #540

lizclipse opened this issue Dec 8, 2022 · 3 comments
Labels
A-async Area: futures, suspense, and async/await S-unactionable Unactionable

Comments

@lizclipse
Copy link

Describe the bug
sycamore::web::render_to_string_await_suspense doesn't implement either Send nor Sync, which is needed for usage in some server contexts.

To Reproduce

use std::net::SocketAddr;

use axum::{
    debug_handler,
    response::{Html, IntoResponse},
    Router,
};
use sycamore::view;

#[tokio::main]
async fn main() {
    let app = Router::new().fallback(ui_render);
    let addr = SocketAddr::from(([127, 0, 0, 1], 3000));
    axum::Server::bind(&addr)
        .serve(app.into_make_service())
        .await
        .unwrap();
}

#[debug_handler]
async fn ui_render() -> impl IntoResponse {
    let ui = sycamore::web::render_to_string_await_suspense(move |cx| {
        view! { cx,
            p { "Hello World" }
        }
    })
    .await;
    Html(ui)
}

Expected behavior
I would expect this function to implement these traits so that it can be neatly and cleanly used in web servers that take advantage of threading for the task executor.

Environment

  • Sycamore: 0.8.2
  • Browser: N/A
  • OS: macOS

Additional context
I've had a look at how you would go about making the function implement these traits, and it's not pleasant. Glancing at the SSR implementation, I've found too many places where !Send or !Sync things are used, so it looks like it'd need a significant rework and I'm honestly not sure if it's super worth it (especially with the added overhead it would introduce).

There are ways to get about this using a LocalPoolHandle from tokio-utils, so it certainly isn't a blocker, but I'm not super certain that is the best way to get around it (async Rust is something I'm still ramping up on). If it's not worth reworking the SSR internals to make it all nice and Snync (send sync), then there should at least be some docs saying that it doesn't implement these traits and mentioning where to go to get around this fact.

Example workaround:

use std::net::SocketAddr;

use axum::{
    debug_handler,
    response::{Html, IntoResponse},
    Router,
};
use sycamore::view;
use tokio_util::task::LocalPoolHandle;

#[tokio::main]
async fn main() {
    let app = Router::new().fallback(ui_render);
    let addr = SocketAddr::from(([127, 0, 0, 1], 3000));
    axum::Server::bind(&addr)
        .serve(app.into_make_service())
        .await
        .unwrap();
}

#[debug_handler]
async fn ui_render() -> impl IntoResponse {
    let pool = LocalPoolHandle::new(1);
    let ui = pool
        .spawn_pinned(|| {
            sycamore::web::render_to_string_await_suspense(move |cx| {
                view! { cx,
                    p { "Hello World" }
                }
            })
        })
        .await
        .unwrap();
    Html(ui)
}
@lukechu10
Copy link
Member

Yeah this one is tricky. I don't think it's even possible to have Send + Sync because the whole reactivity system is inherently single-threaded.

Even if we were to make everything related to SsrNode implement Send + Sync, the reactivity system would still prevent us from making the whole thing Send + Sync.

I think for now, the best is just to stick to using LocalPool from tokio or something similar. We can definitely experiment with converting everything to Send + Sync but I'm not sure if the result would be any faster than it currently is.

@lukechu10 lukechu10 added the A-async Area: futures, suspense, and async/await label Dec 9, 2022
@arctic-hen7
Copy link
Contributor

I may be wrong, but I wonder if this might have something to do with why Suspense panics in the Perseus build process (which is heavily multithreaded)...

@lukechu10
Copy link
Member

If you are talking about framesurge/perseus#190, that would be it. However, Suspense just uses spawn_local_scoped under the hood and nothing else so if that works, then Suspense should work fine as well...

@lukechu10 lukechu10 added the S-unactionable Unactionable label Dec 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async Area: futures, suspense, and async/await S-unactionable Unactionable
Projects
None yet
Development

No branches or pull requests

3 participants