Graceful shutdown #30

nWacky · 2019-06-18T16:19:33Z

Graceful Shutdown

Resolves #29

Synopsis

Add shutdown all connections and remove all data from db when a program receives SIGHUP, SIGINT, SIGTERM, SIGQUIT.

Checklist

alexlapa

@nWacky ,

Тесты не компилируются.

nWacky · 2019-06-19T14:31:45Z

@alexlapa,

Пересмотрел некоторые вещи еще раз, сервер (actix_web::HttpServer) выключается graceful только при SIGTERM

nWacky · 2019-06-20T13:30:25Z

@alexlapa,
Отписался о проблеме закрытия приложения в issue #29

nWacky · 2019-06-21T16:13:51Z

Обсудили, решили сделать следующее:

Описание

Класс GracefulShutdown, который обрабатывает signal::Signal и отправляет всем подписавшимся сообщение о shutdown.

Детали

Адреса подписавшихся хранятся в BTreeMap<u8, Vec<Recipient<...>>.

Чем меньше число приоритета, тем выше приоритет. 0 - самый высокий приоритет.

При получении signal::Signal (кроме SIGCHILD) класс начинает отправлять всем сигналы:

Из Recipient каждой итерации BTreeMap создаем большую future (из адресов для отправки), с помощью join.
После этого (and_then) выполняем все для следующей итерации.

После того, как создали эту большую future, select её с timeout_shutdown (или другим образом даём ей timeout_shutdown времени на завершение) и выключаем систему

Например,
send_0_0.join(send_0_1.join(send_0_2))
.and_then(send_1_0.join(send_1_1))
.and_then(send_2_0)
.select(shutdown_timer)

Для выполнения futures внутри Actor рекомендуется использовать асинхронный контекст Actor.
Использовать Actor message с адресом и приоритетом для подписки.

alexlapa

Тыц. Но, там не совсем все верно, требуется обсудить повторно.

nWacky · 2019-06-25T08:26:18Z

@alexlapa,

Есть еще одна проблема: логгер в actix web паникует пока выключается actix_web:

No logger set. Use `slog_scope::set_global_logger` or `slog_scope::scope`

Из-за этого вся система actix зависает, невозможно никаким образом что-либо сделать (в том числе и таймаут зависает в текущем или если его вынести в другой actor).
Основная цель закрытия - закрыть соединения с кодом 1000 и выключить систему - выполняется успешно, если отключить graceful shutdown для actix_web.

nWacky · 2019-07-12T15:19:54Z

Обсудили, решили следующее:

Убрать RoomSetStopped и изменять состояние с помощью ActorFuture контекста (пример в session.rs)
Вынести CloseRoom в отдельную функцию, чтобы не отправлять сообщения самому себе

nWacky · 2019-07-15T09:57:24Z

@alexlapa, @tyranron,

Убрать RoomSetStopped и изменять состояние с помощью ActorFuture контекста (пример в session.rs)

Если использовать контекст Actora в Futures, то тогда надо посылать и обрабатывать ActorFuture вместо Future -> невозможно использовать join_all().

Я не нашел как сделать из ActorFuture любое dyn Future<Item = (), Error = ()> + Send

tyranron · 2019-07-15T10:33:57Z

@nWacky why? Recipient::send() returns Future, not ActorFuture.

nWacky · 2019-07-15T11:00:27Z

@tyranron,

I tried using it this way:

fn get_drop_fut(
        &mut self,
        ctx: &mut Context<Room>,
    ) -> Box<dyn Future<Item = (), Error = ()> + Send> {
        info!("Closing Room [id = {:?}]", self.id);
        self.state = RoomState::Stopping;
        let drop_fut =
            wrap_future(
                self.participants.drop_connections(ctx)
            ).then(move |_, room: &mut Room, _| {
                room.state = RoomState::Stopped;
                futures::future::ok(())
            });
        Box::new(drop_fut)
    }

get_drop_fut is called whenever we need to get future to close Room (we need this future twice). After drop_connections is called I need to set self.state to Stopped. If I am using example above, medea does not compile due to drop_fut being ActorFuture, not Future. And I don't know how to convert it to ActorFuture

For now the state is being changed with recipient.send(RoomSetStopped {})

EDIT: I tried using Arc and Weak with regular future, but they don't want to be sent around because of in let room_arc = Arc::new(self); self is a non-static reference

nWacky · 2019-07-15T11:50:50Z

Обсудили, решили следующее:

В shutdown'е join_all применять к recipient.send(..)
В Handlerах Actorов использовать асинхронный контекст (если необходимо)
ShutdownMessageResult = Result<(),()>

# Conflicts: # src/api/client/server.rs # src/main.rs # src/turn/service.rs

tyranron

The implementation is too "dirty". A lot of unnecessary changes were introduced (like Send bounds, unremoved CloseRoom message, etc). The documentation was mostly missing.

You should put much more effort to the quality of your implementations in future.

At the moment, I've refactored it. Consider the changes carefully, and do the final testing on the implementation.

FCM:

Implement graceful shutdown (#30, #29)

- impl GracefulShutdown service which listens to OS signals and shutdowns each component gracefully
- impl graceful shutdown for Room and HTTP server
- add [shutdown] config section

Additionally:
- provide explicit states for Room

Be careful with merging and specifying FCM in the Github form:

First line of FCM should go as a title.
Everything other as a body.

Do not forget to remove WIP: prefix.

tyranron · 2019-07-17T08:21:02Z

Cargo.toml

-actix = "0.7"
-actix-web = "0.7"
+actix = "0.8"
+actix-rt = "0.2.3"


Why do you specify the patch version when all the other deps specify minor version only?

tyranron · 2019-07-17T08:25:13Z

src/conf/shutdown_config.rs

+
+#[derive(Clone, Debug, Deserialize, Serialize, SmartDefault)]
+#[serde(default)]
+pub struct ShutdownConfiguration {


Like was said earlier: just Shutdown is enough here. No sense to specify Configuration term as you are in conf module already. You had the similar declarations nearby, don't the difference confuse you?

tyranron · 2019-07-17T08:30:02Z

src/conf/shutdown_config.rs

+#[serde(default)]
+pub struct ShutdownConfiguration {
+    #[default(5000)]
+    pub timeout: u64,


Documentation is mandatory.

tyranron · 2019-07-17T09:04:04Z

src/shutdown/mod.rs

+
+use crate::log::prelude::*;
+
+#[derive(Ord, PartialOrd, PartialEq, Eq, Copy, Clone)]


Alphabetical order by default should be applied.

tyranron · 2019-07-17T11:53:44Z

src/shutdown/mod.rs

+        info!("Exit signal received, exiting");
+
+        if self.recipients.is_empty() {
+            error!("GracefulShutdown: No subscribers registered");


Again, why is this an error? It's a totally normal situation.

tyranron · 2019-07-17T15:30:55Z

src/signalling/participants.rs

    pub fn drop_connections(
        &mut self,
        ctx: &mut Context<Room>,
-    ) -> impl Future<Item = (), Error = ()> {
+    ) -> Box<impl Future<Item = (), Error = ()> + Send> {


Either you use impl Future, or Box<dyn Future>.

Box<impl Future> is unlikely what you want.

tyranron · 2019-07-17T16:20:43Z

src/signalling/room.rs

+#[derive(Message)]
+#[rtype(result = "()")]
+#[allow(clippy::module_name_repetitions)]
+struct RoomSetStopped {}


What is this Message for? I didn't found any usages of it.

@tyranron,

It was used to set Room's state to Stopped back when I was returning futures from ShutdownGracefully because I couldn't modify self from these futures

There is no use for it now

alexlapa · 2019-07-17T20:11:24Z

src/main.rs

+                                     API HTTP server: {}",
+                                    e
+                                )
+                            })


@tyranron,

Напрягает толстый main. main, в моем представлении, должен просто собирать контекст. Предлагаю инжектить
graceful_shutdown в new всех интересующих нас компонентов и там уже подписываться.

Тут, теоретическим минусом может быть то, что тогда Room::new будет рурму не только собирать, но и стартовать. Но я не думаю что это будет проблемой.

Если не так, то тогда хотя бы какой-нибудь helper запилю, дабы кода в main было чуть меньше.

@alexlapa let's refactor it later. Things will be much simple with async/.await usage. And the subscribing itself will be moved to Actor initialization for each kind of long-living thing. But for now just left it as is...

# Conflicts: # Cargo.lock # config.toml # src/api/client/server.rs # src/conf/mod.rs

nWacky added 3 commits June 17, 2019 16:23

fix server address

c6bff7f

add SIGINT handler to ws_connection

ac68159

add exit signals handlers to Room

e9482c3

nWacky added feature New feature or request k::api Related to API (application interface) labels Jun 18, 2019

nWacky added this to the 0.2.0 milestone Jun 18, 2019

nWacky self-assigned this Jun 18, 2019

fix warnings

b1b46fe

nWacky requested a review from alexlapa June 18, 2019 16:39

alexlapa suggested changes Jun 19, 2019

View reviewed changes

nWacky added 2 commits June 19, 2019 15:38

fix tests

cc932cb

format

b5ceb0f

nWacky requested a review from alexlapa June 19, 2019 14:33

minor refactor

26fcfa1

implement graceful_shutdown actor

86ff2af

alexlapa suggested changes Jun 21, 2019

View reviewed changes

nWacky added 5 commits June 21, 2019 19:22

GracefulShutdown prototype implementantion

a7141e0

GracefulShutdown use async actor context

b0d908f

implement then_all

6ae350c

change actix server signals that they wait, encapsulate ServerWrapper

68c3802

remove actix web graceful shutdown

eac51b1

nWacky added 5 commits June 25, 2019 12:20

make then_all poll call recursive

5d89b3b

fix warnings

74cdb77

format

08096fd

send message to subscribe to graceful shutdown

04eeb57

fix test warning

77c40eb

add description

959a4d4

start implementing better signals stream

9a9ca13

move CloseRoom to a function in Room, fix warnings

bb3587e

nWacky added 4 commits July 15, 2019 15:09

start using async actor context

ec2a4dc

refactor

f29e47a

format, lint

0af073f

format

36fdcf7

nWacky requested a review from tyranron July 15, 2019 14:01

tyranron added 4 commits July 17, 2019 11:16

Merge branch 'master' into 29-graceful-shutdown

8ada2fd

# Conflicts: # src/api/client/server.rs # src/main.rs # src/turn/service.rs

Correct conf implementation

f060d62

Refactor shutdown service implementation

8bf9fec

Refactor shutdown handling

ffc0534

tyranron approved these changes Jul 17, 2019

View reviewed changes

tyranron requested a review from alexlapa July 17, 2019 17:14

fix docs

bded4b5

alexlapa reviewed Jul 17, 2019

View reviewed changes

alexlapa added 2 commits July 18, 2019 13:39

Merge branch 'master' into 29-graceful-shutdown

e411747

# Conflicts: # Cargo.lock # config.toml # src/api/client/server.rs # src/conf/mod.rs

fmt && update locks

6c7cada

alexlapa marked this pull request as ready for review July 18, 2019 16:01

alexlapa changed the title ~~WIP: Graceful shutdown~~ Graceful shutdown Jul 18, 2019

alexlapa modified the milestones: 0.2.0, 0.1.0 Jul 18, 2019

alexlapa merged commit 9fa4b5b into master Jul 18, 2019

alexlapa mentioned this pull request Jul 18, 2019

Graceful shutdown #29

Closed

tyranron deleted the 29-graceful-shutdown branch July 18, 2019 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Graceful shutdown #30

Graceful shutdown #30

nWacky commented Jun 18, 2019 •

edited by alexlapa

Loading

alexlapa left a comment

nWacky commented Jun 19, 2019

nWacky commented Jun 20, 2019 •

edited

Loading

nWacky commented Jun 21, 2019 •

edited

Loading

alexlapa left a comment •

edited

Loading

nWacky commented Jun 25, 2019 •

edited

Loading

nWacky commented Jul 12, 2019 •

edited

Loading

nWacky commented Jul 15, 2019

tyranron commented Jul 15, 2019 •

edited

Loading

nWacky commented Jul 15, 2019 •

edited

Loading

nWacky commented Jul 15, 2019

tyranron left a comment •

edited

Loading

tyranron Jul 17, 2019

tyranron Jul 17, 2019

tyranron Jul 17, 2019

tyranron Jul 17, 2019

tyranron Jul 17, 2019

tyranron Jul 17, 2019

tyranron Jul 17, 2019

nWacky Jul 18, 2019 •

edited

Loading

alexlapa Jul 17, 2019 •

edited

Loading

tyranron Jul 18, 2019


		use crate::log::prelude::*;

		#[derive(Ord, PartialOrd, PartialEq, Eq, Copy, Clone)]

Graceful shutdown #30

Graceful shutdown #30

Conversation

nWacky commented Jun 18, 2019 • edited by alexlapa Loading