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

Make actix-rt optional for actix-server #266

Closed
wants to merge 63 commits into from

Conversation

fakeshadow
Copy link
Contributor

@fakeshadow fakeshadow commented Feb 3, 2021

PR Type

Refactor

PR Checklist

Check your PR fulfills the following:

  • Documentation comments have been added / updated.
  • Format code with the latest stable rustfmt

Overview

Make actix-server start by itself regardless the runtime it starts in.

Optional set up actix-system when given.

Remove !Send bind to server startup related future. server handle for sending command must be obtained explicitly with Server::handle API.

actix-server/src/accept.rs Outdated Show resolved Hide resolved
@fakeshadow fakeshadow marked this pull request as draft February 5, 2021 01:59
@fakeshadow
Copy link
Contributor Author

All calls to actix::System has been changed to use try_current. There will be runtime panic thrown when user calls to it if no system provided.

@fakeshadow
Copy link
Contributor Author

fakeshadow commented Mar 28, 2021

These cases are working for now

tokio 1.

use actix_web::{get, App, HttpServer, web};
use actix_web::rt::System;
use tokio::runtime::Handle;

#[tokio::main]
async fn main() -> std::io::Result<()> {
    HttpServer::new(move || App::new().service(index))
        .bind("127.0.0.1:8080")?
        .run().await
}

#[get("/")]
async fn index(handle: web::Data<Handle>) -> &'static str {
    // no system arbiter.
    assert!(System::try_current().is_none());

    // can spawn on tokio runtime.
    let _ = tokio::spawn(async {
        tokio::task::yield_now().await;
    }).await;

    "Hello World!"
}

actix-rt 2.1. Nothing to say.

use actix_web::{get, App, HttpServer};
use actix_web::rt::System;

#[actix_web::main]
async fn main() -> std::io::Result<()> {
    HttpServer::new(|| App::new().service(index))
        .bind("127.0.0.1:8080")?
        .run().await
}

#[get("/")]
async fn index() -> &'static str {
    // have system arbiter.
    assert!(System::try_current().is_some());
    "Hello World!"
}

async-std 1.9. No idea if spawn called in index function share the same runtime context as main. No runtime error though.

use actix_web::{get, App, HttpServer};
use actix_web::rt::System;

#[async_std::main]
async fn main() -> std::io::Result<()> {
    let server = HttpServer::new(|| App::new().service(index))
        .disable_signals()
        .bind("127.0.0.1:8080")?
        .run();

    let handle = server.handle();
    let join = async_std::task::spawn(server);

    // found no easy signal for async std.
    // use a tokio one and tokio1 feature of async-std for easy signal.
    tokio::signal::ctrl_c().await?;

    handle.stop(false).await;
    join.await
}

#[get("/")]
async fn index() -> &'static str {
    // no system arbiter.
    assert!(System::try_current().is_none());

    let _ = async_std::task::spawn(async {
        async_std::task::yield_now().await;
    }).await;

    "Hello World!"
}

futures 0.3. Also no signal.

use actix_web::{get, App, HttpServer};

#[get("/hi")]
async fn hi() -> &'static str {
    "Hello World!"
}

fn main() -> std::io::Result<()> {
    futures::executor::block_on(async {
        HttpServer::new(|| App::new().service(hi))
            .disable_signals()
            .bind("127.0.0.1:8080")?
            .run()
            .await
    })
}

@fakeshadow fakeshadow changed the title simplify server futures. reduce spawned tasks Make actix_rt optional for actix-server Apr 2, 2021
@fakeshadow fakeshadow changed the title Make actix_rt optional for actix-server Make actix-rt optional for actix-server Apr 2, 2021
@fakeshadow fakeshadow marked this pull request as ready for review April 17, 2021 11:22
@fakeshadow
Copy link
Contributor Author

Some interesting bench

use std::io;

use actix_rt::net::TcpStream;
use actix_server::Server;
use actix_service::fn_service;
use tokio::io::AsyncWriteExt;

const BUF: &[u8] = b"HTTP/1.1 200 OK\r\n\
    content-length: 12\r\n\
    connection: close\r\n\
    date: Thu, 01 Jan 1970 12:34:56 UTC\r\n\
    \r\n\
    Hello World!";

#[tokio::main]
async fn main() -> io::Result<()> {
    std::env::set_var("RUST_LOG", "actix=trace");
    env_logger::init();

    let name = "short_life";
    let addr = "127.0.0.1:8080";

    Server::build()
        .bind(name, addr, || fn_service(response))?
        .run()
        .await
}

async fn response(mut stream: TcpStream) -> io::Result<()> {
    stream.write(BUF).await?;
    stream.flush().await?;
    stream.shutdown().await
}

Result:

Running 10s test @ http://localhost:8080/
  12 threads and 512 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     3.32ms    3.43ms  46.71ms   87.85%
    Req/Sec    12.72k     1.49k   23.76k    77.28%
  1519382 requests in 10.10s, 155.04MB read
Requests/sec: 150437.91
Transfer/sec:     15.35MB

Result when using #[actix-rt::main]:

Running 10s test @ http://localhost:8080/
  12 threads and 512 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     2.63ms    4.42ms  67.03ms   88.50%
    Req/Sec    13.24k     3.47k   25.35k    74.28%
  1577444 requests in 10.07s, 160.97MB read
Requests/sec: 156680.57
Transfer/sec:     15.99MB

The result is so close I may switch to use multi-thread tokio from now on.

@fakeshadow fakeshadow marked this pull request as draft May 29, 2021 02:37
@fakeshadow
Copy link
Contributor Author

I don't have time to see through this PR merged. It's basically a complete one that ready to be merged but potential future conflict would not be resolved.
It would be appreciated if someone can take this draft and work it to be merged in the future. The ability to have a smooth switch of runtime would be a good feature to have.

@fakeshadow fakeshadow closed this May 29, 2021
@fakeshadow fakeshadow deleted the refactor/simplify_server_future branch May 30, 2021 17:14
@justintime4tea
Copy link

justintime4tea commented Jun 3, 2021

... potential future conflict would not be resolved.
It would be appreciated if someone can take this draft and work it to be merged in the future...

@fakeshadow
What are you suggesting here? I'm trying to understand what "take this draft and work it to be merged in future" means; is there an action item here someone can pick-up to assist in moving this forward? If so, can we clarify what that/those actions is/are?

You say you're not merging now because of potential future conflict; is part of the "action" needed some form of impl that would "guard" against future conflict happening? I feel I'm missing some context but would like to join the conversation.

I would love to contribute somehow barring I find the time and it's jointly understood what we need to do here.

@fakeshadow
Copy link
Contributor Author

You can check the commit history of this PR and most of them are merging master branch into PR and a lot of times there are conflicts needed resolved for that to happen.

Like my previous reply I don't have time to do this anymore and that's the reason I closed it.

If you want to see it actually merged you are welcomed to copy the changes or make your own implementation for it. It can be another PR but do not expect it can be merged soon.(I don't really know how long that would take though)

@sdykae
Copy link

sdykae commented Jul 23, 2021

any news :(?

@lsunsi
Copy link

lsunsi commented Sep 21, 2021

I was also pretty interested in this PR. Bummed it got closed! I was interested in using the tokio multi threaded runtime instead of the actix-web System as well.

@robjtede robjtede mentioned this pull request Nov 3, 2021
4 tasks
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

Successfully merging this pull request may close these issues.

None yet

5 participants