[GitHub] [airflow] potiuk edited a comment on pull request #22475: Make sure finalizers are not skipped during exception handling

[GitBox <gi...@apache.org>]

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 fork is to exit immediately not giving a chance of any other code to be executed in the fork. This is to emulate Popen() in the place where fork is called. 
   
   If we allow code exceution 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" 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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org