-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix processor cleanup on DagFileProcessorManager #22685
Commits on Apr 1, 2022
-
References to processors weren't being cleaned up after killing them in the event of a timeout. This lead to a crash caused by an unhandled exception when trying to read from a closed end of a pipe.
Configuration menu - View commit details
-
Copy full SHA for fa7712a - Browse repository at this point
Copy the full SHA fa7712aView commit details -
Reap the zombie when killing the processor
When calling `_kill_process()` we're generating zombies which weren't being `wait()`ed for. This led to a process leak we fix by just calling `waitpid()` on the appropriate PIDs.
Configuration menu - View commit details
-
Copy full SHA for 48f0243 - Browse repository at this point
Copy the full SHA 48f0243View commit details
Commits on Apr 2, 2022
-
Reap resulting zombies in a safe way
According to @potiuk's and @malthe's input, the way we were reaping the zombies could cause some racy and unwanted situations. As seen on the discussion over at `https://bugs.python.org/issue42558` we can safely reap the spawned zombies with the changes we have introduced.
Configuration menu - View commit details
-
Copy full SHA for 27502be - Browse repository at this point
Copy the full SHA 27502beView commit details -
Explain why we are actively waiting
As suggested by @potiuk explaining why we chose to actively wait on an scenario such as this one can indeed be useful for anybody taking a look at the code some time from now... Co-authored-by: Jarek Potiuk <jarek@potiuk.com>
Configuration menu - View commit details
-
Copy full SHA for 60f20b8 - Browse repository at this point
Copy the full SHA 60f20b8View commit details -
Fix small typo and triling whitespace
After accepting the changes proposed on the PR we found a small typo (we make those on a daily basis) and a trailing whitespace we though was nice to delete. Hope we made the right choice!
Configuration menu - View commit details
-
Copy full SHA for 2b3eccc - Browse repository at this point
Copy the full SHA 2b3ecccView commit details
Commits on Apr 3, 2022
-
We were calling `poll()` through the `_process` attribute and, as shown on the static checks triggered by GitHub, it's not defined for the `BaseProcess` class. We instead have to call `poll()` through `BaseProcess`'s `_popen` attribute.
Configuration menu - View commit details
-
Copy full SHA for 4a74193 - Browse repository at this point
Copy the full SHA 4a74193View commit details
Commits on Apr 4, 2022
-
References to processors weren't being cleaned up after killing them in the event of a timeout. This lead to a crash caused by an unhandled exception when trying to read from a closed end of a pipe.
Configuration menu - View commit details
-
Copy full SHA for 85f5440 - Browse repository at this point
Copy the full SHA 85f5440View commit details -
Reap the zombie when killing the processor
When calling `_kill_process()` we're generating zombies which weren't being `wait()`ed for. This led to a process leak we fix by just calling `waitpid()` on the appropriate PIDs.
Configuration menu - View commit details
-
Copy full SHA for 9232c2b - Browse repository at this point
Copy the full SHA 9232c2bView commit details -
Reap resulting zombies in a safe way
According to @potiuk's and @malthe's input, the way we were reaping the zombies could cause some racy and unwanted situations. As seen on the discussion over at `https://bugs.python.org/issue42558` we can safely reap the spawned zombies with the changes we have introduced.
Configuration menu - View commit details
-
Copy full SHA for 1f392d6 - Browse repository at this point
Copy the full SHA 1f392d6View commit details -
Explain why we are actively waiting
As suggested by @potiuk explaining why we chose to actively wait on an scenario such as this one can indeed be useful for anybody taking a look at the code some time from now... Co-authored-by: Jarek Potiuk <jarek@potiuk.com>
Configuration menu - View commit details
-
Copy full SHA for 454c383 - Browse repository at this point
Copy the full SHA 454c383View commit details -
Fix small typo and triling whitespace
After accepting the changes proposed on the PR we found a small typo (we make those on a daily basis) and a trailing whitespace we though was nice to delete. Hope we made the right choice!
Configuration menu - View commit details
-
Copy full SHA for 3534263 - Browse repository at this point
Copy the full SHA 3534263View commit details -
We were calling `poll()` through the `_process` attribute and, as shown on the static checks triggered by GitHub, it's not defined for the `BaseProcess` class. We instead have to call `poll()` through `BaseProcess`'s `_popen` attribute.
Configuration menu - View commit details
-
Copy full SHA for ab6555e - Browse repository at this point
Copy the full SHA ab6555eView commit details
Commits on Apr 5, 2022
-
Some changes have been force pushed!
Configuration menu - View commit details
-
Copy full SHA for e3379b1 - Browse repository at this point
Copy the full SHA e3379b1View commit details -
Prevent static check from failing
After reading through `multiprocessing`'s implementation we really didn't know why the static check on line `239` was failing: the process should contain a `_popen` attribute... That's when we found line `223` and discovered the trailing `# type: ignore` comment. After reading up on it we found that it instructs *MyPy* not to statically check that very line. Given we're having trouble with the exact same attribute we decided to include the same directive for the static checker. Hope we made the right call!
Configuration menu - View commit details
-
Copy full SHA for c6ee385 - Browse repository at this point
Copy the full SHA c6ee385View commit details -
Fix test for
_kill_timed_out_processors()
We hadn't updated the tests for the method whose body we've altered. This caused the tests to fail when trying to retrieve a processor's *waitable*, a property similar to a *file descriptor* in UNIX-like systems. We have added a mock property to the `processor` and we've also updated the `manager`'s attributes so as to faithfully recreate the state of the data sctructures at a moment when a `processor` is to be terminated. Please note the `assertions` at the end are meant to check we reach the `manager`'s expected state. We have chosen to check the number of processor's against an explicit value because we're defining `manager._processors` explicitly within the test. On the other hand, `manager.waitables` can have a different length depending on the call to `DagFileProcessorManager`'s `__init__()`. In this test the expected initial length is `1` given we're passing `MagicMock()` as the `signal_conn` when instantiating the manager. However, if this were to be changed the tests would 'inexplicably' fail. Instead of checking `manager.waitables`' length against a hardcoded value we decided to instead compare it to its initial length so as to emphasize we're interested in the change in length, not its absolute value.
Configuration menu - View commit details
-
Copy full SHA for 7f651b6 - Browse repository at this point
Copy the full SHA 7f651b6View commit details
Commits on Apr 6, 2022
-
Fix
black
checks andmock
decoratorsOne of the methods we are to mock required a rather long `@mock.patch` decorator which didn't pass the checks made by `black` on the precommit hooks. On top of that, we messed up the ordering of the `@mock.patch` decorators which meant we didn't set them up properly. This manifested as a `KeyError` on the method we're currently testing. O_o
Configuration menu - View commit details
-
Copy full SHA for 4fc2df3 - Browse repository at this point
Copy the full SHA 4fc2df3View commit details