-
Notifications
You must be signed in to change notification settings - Fork 94
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
ref(actix): Migrate server actor to the "service" arch #1723
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,49 +1,37 @@ | ||
use ::actix::prelude::*; | ||
use actix_web::server::StopServer; | ||
use futures01::prelude::*; | ||
|
||
use relay_config::Config; | ||
use relay_statsd::metric; | ||
use relay_system::{Controller, Shutdown}; | ||
use relay_system::{Controller, Service, Shutdown}; | ||
|
||
use crate::service; | ||
use crate::service::HttpServer; | ||
use crate::statsd::RelayCounters; | ||
|
||
pub struct Server { | ||
http_server: Recipient<StopServer>, | ||
pub struct ServerService { | ||
http_server: HttpServer, | ||
} | ||
|
||
impl Server { | ||
pub fn start(config: Config) -> anyhow::Result<Addr<Self>> { | ||
impl ServerService { | ||
pub fn start(config: Config) -> anyhow::Result<()> { | ||
metric!(counter(RelayCounters::ServerStarting) += 1); | ||
let http_server = service::start(config)?; | ||
Ok(Server { http_server }.start()) | ||
} | ||
} | ||
|
||
impl Actor for Server { | ||
type Context = Context<Self>; | ||
|
||
fn started(&mut self, context: &mut Self::Context) { | ||
Controller::subscribe(context.address()); | ||
let http_server = HttpServer::start(config)?; | ||
Self { http_server }.start(); | ||
Ok(()) | ||
} | ||
} | ||
|
||
impl Handler<Shutdown> for Server { | ||
type Result = ResponseFuture<(), ()>; | ||
|
||
fn handle(&mut self, message: Shutdown, _context: &mut Self::Context) -> Self::Result { | ||
let graceful = message.timeout.is_some(); | ||
|
||
// We assume graceful shutdown if we're given a timeout. The actix-web http server is | ||
// configured with the same timeout, so it will match. Unfortunately, we have to drop any | ||
// errors and replace them with the generic `TimeoutError`. | ||
let future = self | ||
.http_server | ||
.send(StopServer { graceful }) | ||
.map_err(|_| ()) | ||
.and_then(|result| result.map_err(|_| ())); | ||
|
||
Box::new(future) | ||
impl Service for ServerService { | ||
type Interface = (); | ||
|
||
fn spawn_handler(self, _rx: relay_system::Receiver<Self::Interface>) { | ||
tokio::spawn(async move { | ||
let mut shutdown = Controller::shutdown_handle(); | ||
loop { | ||
tokio::select! { | ||
Shutdown { timeout } = shutdown.notified() => { | ||
self.http_server.shutdown(timeout.is_some()); | ||
}, | ||
else => break, | ||
} | ||
} | ||
}); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -4,6 +4,7 @@ use std::time::Duration; | |||
use actix::actors::signal; | ||||
use actix::fut; | ||||
use actix::prelude::*; | ||||
use actix::SystemRunner; | ||||
use futures01::future; | ||||
use futures01::prelude::*; | ||||
use once_cell::sync::OnceCell; | ||||
|
@@ -82,7 +83,7 @@ impl ShutdownHandle { | |||
/// } | ||||
/// | ||||
/// | ||||
/// Controller::run(|| -> Result<(), ()> { | ||||
/// Controller::run(tokio::runtime::Runtime::new().unwrap().handle(), System::new("my-actix-system"), || -> Result<(), ()> { | ||||
/// MyActor.start(); | ||||
/// # System::current().stop(); | ||||
/// Ok(()) | ||||
|
@@ -101,22 +102,29 @@ impl Controller { | |||
SystemService::from_registry() | ||||
} | ||||
|
||||
/// Starts an actix system and runs the `factory` to start actors. | ||||
/// Runs the `factory` to start actors. | ||||
/// | ||||
/// The function accepts the old tokio 0.x [`actix::SystemRunner`] and the reference to | ||||
/// [`tokio::runtime::Handle`] from the new tokio 1.x runtime, which we enter before the | ||||
/// factory is run, to make sure that two systems, old and new one, are available. | ||||
/// | ||||
/// The factory may be used to start actors in the actix system before it runs. If the factory | ||||
/// returns an error, the actix system is not started and instead an error returned. Otherwise, | ||||
/// the system blocks the current thread until a shutdown signal is sent to the server and all | ||||
/// actors have completed a graceful shutdown. | ||||
pub fn run<F, R, E>(factory: F) -> Result<(), E> | ||||
pub fn run<F, R, E>( | ||||
handle: &tokio::runtime::Handle, | ||||
sys: SystemRunner, | ||||
Comment on lines
+116
to
+117
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not create the runtime and the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is because, if I remember it correctly - I use in let runtime = utils::create_runtime("http-server-handler", 1); And it requires that old system is already available at this point. And also I wanted to clean a little bit controller, and make sure that controller knows less about the system it runs on, just accept the system as the parameter. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But isn't spinning up the actix system kind of what the relay/relay-system/src/controller.rs Line 47 in dd9f998
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've changed this doc and added more docs to clarify why it's done this way now. |
||||
factory: F, | ||||
) -> Result<(), E> | ||||
where | ||||
F: FnOnce() -> Result<R, E>, | ||||
F: FnOnce() -> Result<R, E> + 'static, | ||||
F: Sync + Send, | ||||
{ | ||||
let sys = System::new("relay"); | ||||
|
||||
compat::init(); | ||||
|
||||
// Run the factory and exit early if an error happens. The return value of the factory is | ||||
// discarded for convenience, to allow shorthand notations. | ||||
// While starting http server ensure that the new tokio 1.x system is available. | ||||
let _guard = handle.enter(); | ||||
factory()?; | ||||
|
||||
// Ensure that the controller starts if no actor has started it yet. It will register with | ||||
|
@@ -128,7 +136,6 @@ impl Controller { | |||
// until a signal arrives or `Controller::stop` is called. | ||||
relay_log::info!("relay server starting"); | ||||
sys.run(); | ||||
relay_log::info!("relay shutdown complete"); | ||||
|
||||
Ok(()) | ||||
} | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Controller::run()
logs a messagerelay shutdown complete
after returning fromsys.run()
. So by shutting down the tokio runtime withshutdown_timeout
here, do we effectively double the possible shutdown time?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the
tokio docs
:So basically if there is something blocking, we will wait max
shutdown_timeout
and then kill everything.