Replace daemon Thread in dev-mode of breeze start-airflow with forking#31403
Merged
potiuk merged 2 commits intoapache:mainfrom May 19, 2023
Merged
Replace daemon Thread in dev-mode of breeze start-airflow with forking#31403potiuk merged 2 commits intoapache:mainfrom
potiuk merged 2 commits intoapache:mainfrom
Conversation
2 tasks
When you run the dev mode for Breeze start-airlfow the webpack to compile the assets is run in the background and should be stopped as soon as the main process leaves. This has been done via daemon Thread, however it turns out that when daemon thread gets stopped at exit, it is stopped pretty abruptly. Quoting https://docs.python.org/3/library/threading.html :: > Note Daemon threads are abruptly stopped at shutdown. Their resources (such as open files, database transactions, etc.) may not be released properly. If you want your threads to stop gracefully, make them non-daemonic and use a suitable signalling mechanism such as an Event. This also means that if the daemon threds starts a subprocess, the thread is killed but the subprocess is not - the subprocess gets orphaned and gets adopted by init process and continues running. Tis PR replaces the mechanism with forking. Instead of running thread, it will fork a new process that will run the compilation (including starting the subprocess that runs it). It will also change group for the started forked process to be the same as the new pid, which creates a new process group and register an atexit method to send TERM to all the processes belonging to that grop, so even if the node process that runs compilation does not propagate the TERM signal (it does not) to the webpack processes, all the webpack processes share the same process group and get the SIGTERM signal. Fixes: apache#31384
potiuk
force-pushed
the
fix-stopping-webpack-in-dev-mode
branch
from
4cb45fd to
8c33708
Compare
eladkal
approved these changes
May 19, 2023
pierrejeambrun
approved these changes
May 19, 2023
Member
pierrejeambrun
left a comment
pierrejeambrun
left a comment
There was a problem hiding this comment.
Thank you !
just a minor typo to fix, LGTM
Co-authored-by: Pierre Jeambrun <pierrejbrun@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When you run the dev mode for Breeze start-airlfow the webpack to compile the assets is run in the background and should be stopped as soon as the main process leaves. This has been done via daemon Thread, however it turns out that when daemon thread gets stopped at exit, it is stopped pretty abruptly. Quoting
https://docs.python.org/3/library/threading.html ::
This also means that if the daemon threds starts a subprocess, the thread is killed but the subprocess is not - the subprocess gets orphaned and gets adopted by init process and continues running.
Tis PR replaces the mechanism with forking. Instead of running thread, it will fork a new process that will run the compilation (including starting the subprocess that runs it). It will also change group for the started forked process to be the same as the new pid, which creates a new process group and register an atexit method to send TERM to all the processes belonging to that grop, so even if the node process that runs compilation does not propagate the TERM signal (it does not) to the webpack processes, all the webpack processes share the same process group and get the SIGTERM signal.
Fixes: #31384
^ 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.rstor{issue_number}.significant.rst, in newsfragments.