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

build_state_fn doesn't work after perseus::autoserde(build_state) (Sync required) #96

Closed
adundovi opened this issue Dec 21, 2021 · 12 comments

Comments

@adundovi
Copy link
Contributor

Describe the bug

Since the newest beta, or after 0.3.0beta18, I believe, and now with newly released 0.3.0, I encounter the following error when building with perseus:

error: future cannot be shared between threads safely
   --> src/templates/pages/about.rs:23:25
    |
23  |         .build_state_fn(get_build_props)
    |                         ^^^^^^^^^^^^^^^ future returned by `get_build_props` is not `Sync`
    |
    = help: the trait `Sync` is not implemented for `(dyn Future<Output = Result<surf::Response, surf::Error>> + Send + 'static)`
note: future is not `Sync` as this value is used across an await
   --> src/templates/components/markdown.rs:39:23
    |
39  |         let content = surf::get(url).recv_string().await.unwrap();
    |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ first, await occurs here, with `surf::get(url)` maybe used later...
note: `surf::get(url)` is later dropped here
   --> src/templates/components/markdown.rs:39:66
    |
39  |         let content = surf::get(url).recv_string().await.unwrap();
    |                       --------------                             ^
    |                       |
    |                       has type `RequestBuilder` which is not `Sync`
note: required by a bound in `Template::<G>::build_state_fn`
   --> .cargo/registry/src/github.com-1ecc6299db9ec823/perseus-0.3.0/src/template.rs:510:19
    |
510 |         val: impl GetBuildStateFnType + Send + Sync + 'static,
    |                   ^^^^^^^^^^^^^^^^^^^ required by this bound in `Template::<G>::build_state_fn`

I see that the problem is related to surf, but this worked a few versions ago, before the introduction #[perseus::autoserde(build_state)]. How to fix this?

Here is a sample:

#[perseus::template(AboutPage)]
#[component(AboutPage<G>)]
pub fn about_page(props: MarkdownPageProps) -> View<G> {
      view! {
          MainLayout(
              view!{ MarkdownProse(props) }
          )
      }
}
  
pub fn get_template<G: Html>() -> Template<G> {
      Template::new("about")
          .build_state_fn(get_build_props)
          .template(about_page)
          .head(|_| {
            view! {
                  title { "Some title" }
              }
          })
}
  
#[perseus::autoserde(build_state)]
  pub async fn get_build_props(_path: String, locale: String) -> RenderFnResultWithCause<MarkdownPageProps> {
      Ok(MarkdownPageProps::fetch(&format!(
              "(...)/{loc}/about.md",
              loc = locale
          )
      ).await)
  }
#[derive(Serialize, Deserialize, Debug)]
pub struct MarkdownPageProps {
    pub text: String,
}

impl MarkdownPageProps {
    #[cfg(not(target_arch = "wasm32"))]
    pub async fn fetch(url: &str) -> MarkdownPageProps {
        let content = surf::get(url).recv_string().await.unwrap();
        MarkdownPageProps { text: content }
    }

Environment (please complete the following information):

  • Perseus Version: 0.3.0
  • Sycamore Version: 0.7.1
  • OS: GNU/Linux
@arctic-hen7
Copy link
Member

Unfortunately, since the release of the Warp integration, there are stricter requirements on build functions being sendable and syncable (Actix Web does some unsafe magic and so didn't need them). Regrettably, this is a necessity for the Warp integration to work, so you'll have to find a way to work around this with surf.

That said, I'm happy to have a look at this and see if I can find a workaround. Would you mind posting an MRE?

@adundovi
Copy link
Contributor Author

Thanks @arctic-hen7 for a quick reply.

OK, here is an MRE (I'm not sure if it is really minimal, but I built it on top of hello world and the 2nd app):

Cargo.toml:

[package]
name = "mre-surf"
version = "0.1.0"
edition = "2021"

[dependencies]
perseus = { version = "0.3.0", features = [ "hydrate" ] }
sycamore = "0.7"
serde = { version = "1", features = ["derive"] }
serde_json = "1"

[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
surf = "2.3.2"

src/lib.rs:

use perseus::{
    define_app,
    ErrorPages,
    Html,
    RenderFnResultWithCause,
    Template
};
use sycamore::{
    view,
    prelude::{component, View}
};
use serde::{Deserialize, Serialize};

#[derive(Serialize, Deserialize, Debug)]
struct IpAddr {
    ip: String
}

define_app! {
    templates: [
        example_template::<G>()
    ],
    error_pages: ErrorPages::new(|url, status, err, _| {
        view! {
            p { (format!("An error with HTTP code {} occurred at '{}': '{}'.", status, url, err)) }
        }
    })
}

#[perseus::template(IndexPage)]
#[component(IndexPage<G>)]
pub fn index_page(props: IpAddr) -> View<G> {
    view! {
        p { "Your IP is " (props.ip) }
    }
}

pub fn example_template<G: Html>() -> Template<G> {
    Template::new("index")
        .build_state_fn(get_build_props)
        .template(index_page)
}

#[perseus::autoserde(build_state)]
pub async fn get_build_props(_path: String, _locale: String) -> RenderFnResultWithCause<IpAddr> {
      Ok(get_ip().await.unwrap())
}

#[cfg(not(target_arch = "wasm32"))]
pub async fn get_ip() -> Result<IpAddr, surf::Error> {
    let content = surf::get("https://api.ipify.org?format=json")
          .recv_json();
    content.await
}

#[cfg(target_arch = "wasm32")]
pub async fn get_ip() -> Result<IpAddr, Box<dyn std::error::Error>> {
    Ok(IpAddr { ip: "127.0.0.1".to_string() })
}

The wasm version of get_ip is a dummy function, and it is not a problem when I implement the real one based on, for example, reqwasm.

@adundovi adundovi changed the title build_state_fn doesn work after perseus::autoserde(build_state) (Sync required) build_state_fn doesn't work after perseus::autoserde(build_state) (Sync required) Dec 22, 2021
@arctic-hen7
Copy link
Member

arctic-hen7 commented Dec 22, 2021

Great, thanks. I'll look at this in detail first chance I get.

@arctic-hen7
Copy link
Member

Sorry for the delay! My initial work on this suggests that the problem code is the + Sync requirements in this code:

https://github.com/arctic-hen7/perseus/blob/5c338aa646f7abb7cb5751e818439ffb0028d110/packages/perseus/src/template.rs#L137-L157

If these are removed, I think your code would work, but, unfortunately, they're crucial for any multi-threaded servers other than Actix Web (which seems to use unsafe to get this to work). The root cause of the issue given this appears to be that surf::RequestBuilder is Send but not Sync.

Now, given that you're guaranteeing that this version of the get_ip() function will only run at build time, you could use futures::executor::block_on instead of await to circumvent this problem. That makes your code compile, but I can't vouch for the build performance if you wanted to scale this up! You'd end up with something like this:

#[cfg(not(target_arch = "wasm32"))]
pub async fn get_ip() -> Result<IpAddr, surf::Error> {
    let content = surf::get("https://api.ipify.org?format=json")
        .recv_json::<IpAddr>();
    let content = futures::executor::block_on(content); // Works, but terrible for build times in larger apps
    content
}

As for the implications for Perseus, I think this is unfortunately an error that users will have to address. However, I will put this requirement in the common pitfalls section of the docs so that it's easier to understand.

@adundovi
Copy link
Contributor Author

adundovi commented Dec 29, 2021

Dear @arctic-hen7, thank you for your reply and effort. It makes sense what you're suggesting, however, I have the same problem on the client side, too, with reqwasm (actually, wasm_bindgen). But before going into the technical details, I am wondering - I would like to use Perseus simply as a front-end app which fetches some REST API - this should be a quite normal scenario, right? I would like to do that both, on client and on server side. Am I missing something? How do you do that in practice? What fetch libraries do you use? Shouldn't these tasks be trivial to make?

@arctic-hen7
Copy link
Member

They should be. One of the reasons there are issues with this is the lack of a proper async runtime in Perseus for builders, which means blocking libraries like ureq are currently the way to go. In v0.3.1, tokio will be introduced in the builder system, which should make this much easier on the server side.

As for the client, I haven't personally used reqwasm before, I've just depended on the web_sys access to the fetch API. However, I agree that this is a very common use case, so I'm happy to set up an example for fetching data, and I'll try out reqwasm.

@arctic-hen7
Copy link
Member

Okay, as of #102, we have an async runtime! That should make this easier with libraries like reqwest on the server-side, and I'll work on the client-side stuff later today (hopefully there'll be a full example ready in a few hours).

@rieval
Copy link

rieval commented Dec 30, 2021

Okay, as of #102, we have an async runtime! That should make this easier with libraries like reqwest on the server-side, and I'll work on the client-side stuff later today (hopefully there'll be a full example ready in a few hours).

Amazing!

@adundovi
Copy link
Contributor Author

Okay, as of #102, we have an async runtime! That should make this easier with libraries like reqwest on the server-side, and I'll work on the client-side stuff later today (hopefully there'll be a full example ready in a few hours).

Wou! That was fast! Thanks a lot.

Amazing!

Indeed!

arctic-hen7 added a commit that referenced this issue Dec 30, 2021
@arctic-hen7
Copy link
Member

Thank you, no problem!

There's now an example for fetching data in the examples/ directory. Making it, I was reminded of the beautiful thing known as CORS, which now has its own little section in the docs. I wonder if this might have been the cause of your troubles on the client-side?

@rieval
Copy link

rieval commented Dec 30, 2021

Thank you, no problem!

There's now an example for fetching data in the examples/ directory. Making it, I was reminded of the beautiful thing known as CORS, which now has its own little section in the docs. I wonder if this might have been the cause of your troubles on the client-side?

Brave! 🎉
Are there any plans to release a version? I can't wait to try it out!

@arctic-hen7
Copy link
Member

Yeah, I'll get hot reloading working and then v0.3.1 should push in the coming days.

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

No branches or pull requests

3 participants