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

Example for registering data is incomplete and fails in the obvious use case #1033

Closed
Lindenk opened this issue Aug 14, 2019 · 4 comments
Closed

Comments

@Lindenk
Copy link

Lindenk commented Aug 14, 2019

Here's the example for adding application data to the server:

use std::sync::Mutex;
use actix_web::{web, App};

struct MyData {
    counter: usize,
}

/// Use `Data<T>` extractor to access data in handler.
fn index(data: web::Data<Mutex<MyData>>) {
    let mut data = data.lock().unwrap();
    data.counter += 1;
}

fn main() {
    let data = web::Data::new(Mutex::new(MyData{ counter: 0 }));

    let app = App::new()
        // Store `MyData` in application storage.
        .register_data(data.clone())
        .service(
            web::resource("/index.html").route(
                web::get().to(index)));
}

And here is the example for running a server:

fn main() {
    HttpServer::new(|| {
        App::new()
            .route("/", web::get().to(index))
            .route("/again", web::get().to(index2))
    })
    .bind("127.0.0.1:8088")
    .unwrap()
    .run()
    .unwrap();
}

The first example does not include HttpServer, doesn't bind to a port, and doesn't serve an application. Attempting to wrap the App in an HttpServer means web::Data<T> must be Send. Apparently, it also forces T in web::Data<Arc<Mutex<T>>> to also be Send instead of using the wrapped value.

Here's a direct example of the problem:

use std::sync::{Arc, Mutex};
use std::rc::Rc;
use actix_web::{web, App, HttpServer};

fn main() {
  let foo = web::Data::new(Arc::new(Mutex::new(Rc::new(0))));
  
  HttpServer::new( || {
    App::new()
      .register_data(foo.clone())
  })
    .bind("0.0.0.0:8080").unwrap()
    .run().unwrap();
}
@ninjasource
Copy link

ninjasource commented Aug 15, 2019

Same issue here, solution below. This applies to the Shared Mutable State section under Basics | Application in the docs. It is not clear where the user should put the app data.

The example should be something like this (note the "move" when creating the HttpServer):

fn main() {
    let data = web::Data::new(AppState {
        counter: Mutex::new(0),
    });

    HttpServer::new(move || {
        App::new()
            .register_data(data.clone()) // <- register the created data
            .route("/", web::get().to(index))
    })
        .bind("127.0.0.1:1337")
        .unwrap()
        .run()
        .unwrap();
}

Note to reader: See Extractors | Async Data Access section for an explanation about the dangers of using a mutex in an async handler. Your application becomes single threaded while the mutex is held and awaiting inside one of these locks is particularly bad because it can lead to deadlocks.

@leizzer
Copy link
Member

leizzer commented Aug 21, 2019

@Lindenk where is that example from?
did you check the state example?
https://github.com/actix/examples/tree/master/state

Actix-web went through a lot of changes, so some examples could be outdated

@ninjasource
Copy link

Thanks @Lindenk, I figured it out from the state example. This is indeed a case of outdated docs. See
https://actix.rs/docs/application/
In the "Shared Mutable State" section and after the text "and register the data in an App:" the code is incomplete as an example.

@leizzer
Copy link
Member

leizzer commented Aug 22, 2019

I just filled a ticket regarding this documentation issue actix/actix-website#109
Gonna close this issue now

@leizzer leizzer closed this as completed Aug 22, 2019
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