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

Fix triggerer thread crash in daemon mode #34931

Merged
merged 2 commits into from Oct 14, 2023

Conversation

Bisk1
Copy link
Contributor

@Bisk1 Bisk1 commented Oct 13, 2023

Fixes #34816

Change the order of operations so that async child thread is created after forking when entering daemon context. This makes sure that the thread stays alive in the internal loop - forking causes stopping all child threads.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Change the order of operations so that async child thread is created after forking when entering daemon context.

This makes sure that the thread stays alive in the internal loop.
@Bisk1 Bisk1 changed the title Fixes #34816 Fix triggerer thread crash in daemon mode Oct 13, 2023
@Bisk1
Copy link
Contributor Author

Bisk1 commented Oct 13, 2023

Also as explained in #34816 (comment), I'm willing to follow up with another contribution where all other commands are refactored to use the same pattern for daemon mode (and maybe reuse the same code?) but it needs a longer thought so I think it deserves a separate PR.

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! I believe this would fix the issue.

Could you fix the static checks? https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst

@Bisk1
Copy link
Contributor Author

Bisk1 commented Oct 13, 2023

Interesting! I believe this would fix the issue.

Could you fix the static checks? https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst

@hussein-awala done

@eladkal eladkal added this to the Airflow 2.7.3 milestone Oct 14, 2023
@eladkal eladkal added the type:bug-fix Changelog: Bug Fixes label Oct 14, 2023
@potiuk potiuk merged commit 9c1e8c2 into apache:main Oct 14, 2023
43 checks passed
@Bisk1 Bisk1 deleted the fix-triggerer-daemon branch October 14, 2023 16:05
ephraimbuddy pushed a commit that referenced this pull request Oct 29, 2023
* Fixes #34816

Change the order of operations so that async child thread is created after forking when entering daemon context.

This makes sure that the thread stays alive in the internal loop.

---------

Co-authored-by: daniel.dylag <danieldylag1990@gmail.com>
(cherry picked from commit 9c1e8c2)
ephraimbuddy pushed a commit that referenced this pull request Oct 30, 2023
* Fixes #34816

Change the order of operations so that async child thread is created after forking when entering daemon context.

This makes sure that the thread stays alive in the internal loop.

---------

Co-authored-by: daniel.dylag <danieldylag1990@gmail.com>
(cherry picked from commit 9c1e8c2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Airflow 2.7.1 can not start Scheduler & trigger
4 participants