potiuk edited a comment on pull request #22475: URL: https://github.com/apache/airflow/pull/22475#issuecomment-1076828755
> While moving `os._exit()` out of the `finally` block is (I believe) the correct fix, I don’t think those changes to `BaseException` are needed. IMO at least the first one should be reverted. I think it's quite important actually. Here is the inheritance graph of exceptions: https://docs.python.org/3/library/exceptions.html#exception-hierarchy After moving the os._exit() out of `finally`, If any of `GeneratorExit` `KeyboardException`, `SystemExit` are raised in the fork, then `os._exit()` will not be called in the fork. While it is likely not a big issue (those exceptions will be raised further and *probably* not caught and lead to fork process exit), this is not the intention here. I believe the intention of the fork here is that the fork should never, ever leave the scope of the `_start_by_fork` function. The whole idea about `os._exit()` in the fork is to exit immediately not giving a chance of any other code to be executed in the fork. This is to emulate `Popen.run()` in the place where fork is called. If we allow code execution in the fork to go up the stack in here, this is potentially very distrupting. This will mean that fork process **might** potentially start executing a code that was never supposed to be executed in fork - only in the parent process. For example if the parent process does: ```python def run_task(): process = _start_by_fork() wait(process) def do_something(): run_task() try: do_something() except BaseException: do_some_cleanup() ``` If we just do `try/except Exception` inside the fork before `os._exit()` and BaseException happens, then `do_some_cleanup()` will be executed in BOTH - parent process and fork process - which is certainly not intended. I think in such "low-level" code we should catch ALL POSSIBLE exceptions. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
