ref(actix): Migrate server actor to the "service" arch #1723
Conversation
| handle: &tokio::runtime::Handle, | ||
| sys: SystemRunner, |
There was a problem hiding this comment.
Why not create the runtime and the actix::SystemRunner inside run()? Is there a use case where run() recycles an existing runtime?
There was a problem hiding this comment.
This is because, if I remember it correctly - I use in relay-server/src/lib.rs following code
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.
But isn't spinning up the actix system kind of what the Controller is for? Looking at its docs
relay/relay-system/src/controller.rs
Line 47 in dd9f998
There was a problem hiding this comment.
I've changed this doc and added more docs to clarify why it's done this way now.
relay-server/src/service.rs
Outdated
| } | ||
|
|
||
| /// Triggers the shutdown process by sending [`actix_web::server::StopServer`] to the running http server. | ||
| pub fn shutdown(self, graceful: bool) { |
There was a problem hiding this comment.
Why does this take self by value instead of by reference?
There was a problem hiding this comment.
I guess it mostly to signal to the caller that once you can the shutdown, the server is not longer can be used after that.
There was a problem hiding this comment.
The intent makes sense, but wouldn't changing it to &mut self prevent the need to clone here?
There was a problem hiding this comment.
you're right. I've changed it.
| Controller::run(runtime.handle(), sys, || ServerService::start(config))?; | ||
|
|
||
| // Properly shutdown the new tokio runtime. | ||
| runtime.shutdown_timeout(shutdown_timeout); |
There was a problem hiding this comment.
Controller::run() logs a message relay shutdown complete after returning from sys.run(). So by shutting down the tokio runtime with shutdown_timeout here, do we effectively double the possible shutdown time?
There was a problem hiding this comment.
From the tokio docs:
Shuts down the runtime, waiting for at most duration for all spawned task to shutdown.
So basically if there is something blocking, we will wait max shutdown_timeout and then kill everything.
This change introduces changes to migrate `Server` actor to the new `Service` architecture: * remove `actix` dependencies from the `Server` actor * implement `Service` for the `Server` actor * introduce the `HttpServer` helper struct, which takes care of starting the shutting down the `actix_web` http service, and removes exposed `Recipient` from the `Server` actor - now everything is hidden and in the future can be removed from the one place. The former `Server` actor and current `ServerService` subscribes only to `Shutdown` watch channel and makes sure to trigger the shutdown of the http server with all its workers.
* master: (35 commits) ref(actix): Migrate ProjectUpstream to `relay_system::Service` (#1727) feat(general): Add unknown SessionStatus variant (#1736) ref: Convert integration tests about dropping transactions to unit tests (#1720) release: 0.8.16 ci: Skip redundant self-hosted E2E on library release (#1755) doc(changelog): Add relevant changes to python changelog (#1753) feat(profiling): Add profile context (#1748) release: 23.1.0 profiling(fix): use an unpadded base64 encoding (#1749) Revert "feat(replays): Enable PII scrubbing for all organizations" (#1747) feat: Switch from base64 to data-encoding (#1743) instr(replays): Add timer metric to recording processing (#1742) feat(replays): Use Annotated struct definition for replay-event parsing (#1582) feat(sessions): Retire session duration metric (#1739) feat(general): Scrub all fields with IP address (#1725) feat(replays): Enable PII scrubbing for all organizations (#1678) chore(project): Add backoff mechanism for fetching projects (#1726) feat(profiling): Add new measurement units for profiling (#1732) chore(toolchain): update rust to 1.66.1 (#1735) ref(actix): Migrate server actor to the "service" arch (#1723) ...
This change introduces changes to migrate
Serveractor to the newServicearchitecture:actixdependencies from theServeractorServicefor theServeractorHttpServerhelper struct, which takes care of starting the shutting down theactix_webhttp service, and removes exposedRecipientfrom theServeractor - now everything is hidden and in the future can be removed from the one place.The former
Serveractor and currentServerServicesubscribes only toShutdownwatch channel and makes sure to trigger the shutdown of the http server with all its workers.These changes are still up to discussions.
closes: #1606
#skip-changelog