HH-217737 fix fork workers race conditions #727
Conversation
frontik/app.py
Outdated
| if self.worker_state.single_worker_mode: | ||
| self.worker_state.master_done.value = True | ||
| def deinit(self) -> None: | ||
| if isinstance(self.service_discovery, MasterServiceDiscovery): |
There was a problem hiding this comment.
это ж можно на уровне реализаций разрулить?
frontik/service_discovery.py
Outdated
| app_name: str, | ||
| ) -> None: | ||
| self.with_consul: bool = with_consul | ||
| class MasterServiceDiscovery: |
There was a problem hiding this comment.
а почему не стал делать интерфейс который оба класса имплементят?
There was a problem hiding this comment.
пересечение маленькое
сделал ща, можешь посмотреть как получилось
frontik/app.py
Outdated
| with_consul = self.worker_state.single_worker_mode and options.consul_enabled | ||
| self.make_service_discovery(self.worker_state.init_shared_data, with_consul) | ||
| if isinstance(self.service_discovery, MasterServiceDiscovery): | ||
| self.service_discovery.register_service() |
There was a problem hiding this comment.
и тут можно унести if на уровень реализации
frontik/process.py
Outdated
| resend_dict: dict = field(default_factory=lambda: {}) # pid: flag | ||
| terminating: bool = False | ||
| single_worker_mode: bool = True | ||
| init_shared_data: dict = field(default_factory=lambda: {}) |
frontik/app.py
Outdated
| self.asgi_app.http_client_factory = self.http_client_factory | ||
|
|
||
| if self.worker_state.single_worker_mode: | ||
| self.worker_state.master_done.value = True |
There was a problem hiding this comment.
а что это вообще значит - master_done?
вот там выше в fork-функции тоже проставляется True, какая логика?
There was a problem hiding this comment.
в системной ручке /статус проверка что воркеры стартанули и мастер отработал
это флаг мастера
тут ставится если синглпроцесс режим
712u3
force-pushed
the
HH-217737
branch
2 times, most recently
from
03f1731
to
993aa92
Compare
frontik/app.py
Outdated
| request_balancer_builder = RequestBalancerBuilder( | ||
| self.upstream_manager.get_upstreams(), | ||
| self.service_discovery.get_upstreams_unsafe(), # very very bad |
There was a problem hiding this comment.
в названии?
ну потому что мы выдаем ссылку на священные мутабельные апстримы и если кто-то их поменяет то может клиент сломаться который при каждом запросе по ним бегает
или он сам их поменяет 😱
There was a problem hiding this comment.
а нельзя какие-то потокобезопасные коллекции использовать или обернуть методы в локи?
что интересно, при отказе от gil они хотят явно сделать некоторые операции потокобезопасными: python/cpython#112075 (сейчас, насколько я понимаю, эти операции являются безопасными неявно из-за gil)
712u3
force-pushed
the
HH-217737
branch
2 times, most recently
from
1fc9068
to
10ebf14
Compare
frontik/handler.py
Outdated
| @@ -424,6 +424,10 @@ async def _execute_page(self) -> None: | |||
| assert self.route.dependant.call is not None | |||
| returned_value: ReturnedValue = await self.route.dependant.call(**values) | |||
|
|
|||
| # It is very important to set f_request=None, without this the variable will be deleted only at the next gc call | |||
There was a problem hiding this comment.
тока не при следующем gc, а при выходе из метода, потому что питон решит, что значение больше не нужно.
не хочешь f_request подвинуть выше? или он ничего тяжёлого, кроме self, не держит? или values внутри всё равно на f_request ссылается?
There was a problem hiding this comment.
выше плохо да, он нужен в валуесах и в route.dependant.call
There was a problem hiding this comment.
вроде правильно написал
"ставим ноне, тк если не поставим то только на некст гц удалится"
https://jira.hh.ru/browse/HH-217737