sals.process.kill function is not efficient and returns True even the Process still a live #557
Comments
sameh-farouk
added a commit
that referenced
this issue
Feb 1, 2021
- Fixing issue #557. - Adding optional args timeout, sure_kill. - dealing with edge cases and ensure proper error handling. - updating the docstring.
abom
added a commit
that referenced
this issue
Mar 11, 2021
* refactor: remove unused import statements * chore: Commenting the code * docs: Update excute() docstring - Matching google python docstring style - Adding missing info for cwd arg * refactor: Refactoring is_alive() & update its docstring - Removing Redundant type check - Updating the docstring, matching google style * refactor: Refactoring is_installed() - Update docstring to match google style - simplify the code * fix: Re-implementing kill() to fix a bug -> #557 - Fixing issue #557. - Adding optional args timeout, sure_kill. - dealing with edge cases and ensure proper error handling. - updating the docstring. * refactor: Refactoring get_pids() - Adding new optional args limit, _alt_source - Changing the default_predicate operator to equality instead of contain - check against the exe and command basename not the whole command path - updating and restyling the docstring * refactor: Refactoring get_process_object - Restyling the docstring to match google style - Catching psutil.AccessDenied error * docs: Restyling the docstring to match google style * refactor: Refactoring check_running() - using new get_pids `limit` option - removing redundant code and simplify the code - Restyling the docstring to match google style * docs: Updating check_running docsring * refactor: Refactoring check_start() - Making use of check_running() - Adding proper error handling - remove some make-no-sense code - use psutil.Popen to execute the cmd for a better control of the executed subprocess - Updating and restyling docstring * refactor: Refactoring check_start() - Making use of check_running() - Adding proper error handling - remove some make-no-sense code - use psutil.Popen to execute the cmd for a better control of the executed subprocess - Updating and restyling docstring * refactor: Refactoring check_stop() - Adding proper error handler - Use psutil.Popen for better control of the executed subprocess - Updating the docstring and restyling it to match google style * refactor: Refactoring ps_find() - making use of get_pids() `limit` new option to make the function faster - Updating the docstring * refactor: Refactoring kill_all() - making use of kill_process_by_name() due to the similarities. - Updating the docstring * refactor: Refactoring kill_process_by_name() - Adding new optional args 'timeout' and 'sure_kill' - Making use of kill() 'timeout' and 'sure_kill' - Restyling the docstring * refactor: Re-implementing get_defunct_processes() - getting rid of `ps` shell command replacing it with psutil - updating the docstring * refactor: Refactoring is_port_listening() - Making use of nettools sal - Adding support for ports bound to IPv6 localhost address using a new optional arg `ipv6` - Updating the docstring * refactor: Refactoring get_user_processes() - Adding proper error handling - Now it yields psutil.Process instead of retuning the pid(s) list for faster reusing between module functions and for consistency sake. - Updating the docstring * refactor: Refactoring get_similar_processes() - Adding proper error handling - Now it Yielding the processes - Updating the docstring * refactor: Refactoring kill_user_processes() - Making use of kill() `sure_kil` new option - Updating the docstring * refactor: Refactoring check_process_for_pid() - Adding proper error handling - Updating the docstring * refactor: Refactoring set_env_var() - renaming args to more readable names. - Updating the docstring. * fix: Re-implementing get_process_by_port() issue #561 - Re-implementing the function to be more generic - Adding new args `ipv6`, `udp` - fix issue # 561 - Updating the docstring * refactor: Refactoring get_pid_by_port - Adding optional args and supporting for `ipv6` and `udp` - Making use of get_pid_by_port() new args `ipv6` and `udp` * docs: Updating the docstring * docs: Updating the docstring * refactor: Refactoring kill_process_by_port - Making use of get_process_by_port() and its new args - Making use of kill() new args `timeout` and `sure_kill` * docs: Updating get_processes() docstring * docs: Updating get_ports_mapping() docstring * docs: Updating get_memory_usage() docstring * refactor: Refactoring get_environ() - Adding proper error handling - remove redundant type check * feature: Re-implementing get_processes_info() -> #467 - Making the function generic with adding args for filter, sort and limit. - Updating the docstring * refactor: Refactoring get_pids_filtered_by_regex() - increase function performance using `attrs` process_iter() arg - simplifying the code - Updating the docstring * refactor: Re-implementing get_pids_filtered_sorted() -> issue #467 - Making use of get_processes_info() to replace `ps` command - Updating the docstring * refactor: Re-implementing get_filtered_pids() - Replacing `ps` shell command with psutil - Update the docstring * docs: Updating the docstr * refactor: sorting imports and remove the unused * docs: Updating the module docstring and outdated examples * tests: Updating few tests to keep up with the sal changes * fix: Retunning True from kill() when the process no longer exists * feature: Skipping errors when killing multiple Processes - now kill_all(), kill_process_by_name(), kill_user_processes() will not raise exception when one of the processes fail to terminate, instead they will skip any errors and at the end, return a list of the processes IDs that still alive. * feat: Adding logging to kill() * docs: Upadting kill_all() and kill_process_by_name() docstrings * docs: Upadting get_pid_by_port() and get_process_by_port() docstrings * feat: Adding new function kill_proc_tree() - the module already contain other function that kill all processes that match a process name, but kill_proc_tree() will kill all processes that descendant from a given process regardless the name. so if you want to get a rid of a process and all subprocess that could have been spawn from the that process, its now possible using this function. - making use of kill() and its new args `timeout` and `sure_kill` * docs: Updating get_pids_filtered_sorted() docstring * feat: Adding logging to get_filtered_pids() * test: Updating get_user_processes() test - Updating the test needed as the function now return a generator object instead of a list * test: Updating get_user_processes() test * feat: Adding logging to get_user_processes() * feat: Adding logging to check_start() * docs: Updating kill_proc_tree() docstring * refactor: modifiy kill_user_processes() to return Process objects * fix: Fixing get_processes_info() returns None * fix: Restoring accidentally deleted? functionality for get_processes_info() * feat: Adding logging to check_stop() * tests: Adding debuging code in test_process.py * Adding Debug code investigating a build issue * fix: Fixing get_pids_filtered_sorted return process objects not list[int] * fix: Fixing AttributeError raised from get_pids_filtered_sorted() * docs: Updating get_processes_info() docstring * fix: Updating default_predicate() * fix: Updating get_pids() * docs: Re-generating docs * fix: Fixing test_22_get_sorted_pids * Revert "fix: Updating default_predicate()" This reverts commit aae8120. * tests: Fixing bug in test_process.get_process_pids() - it was getting more processes than those actually running - running sudo tail for example, and searching for `tail` instances will return two pids, one for `sudo` and one for `tail` because both have `tail` string in the command line. - now it use `pidof` instead of `ps` and `grep` to retrieve accurate results. * Update test_process.py * tests: Fixing tests * tests: revert last merge * tests: Fixing tests * tests: Fixing typo * tests: Fixing bug in getting username * feat: get_pids now ignoring zombie processes by default * fix: change get_pids() default sort direction to Asc * docs: Updating get_pids() docstring * tests: Fixing Defunct processes are reported with pgrep. * tests: Fixing tests * tests: Fixing getting current user name (in portable way) * Adding a new arg `full_cmd_line` to get_pids() - The pattern was only matched against the process name, this commit allowing the full command line is used using a new arg 'full_cmd_line' - Updating the docstring * tests: fixing test_12_get_processes_info * fix: Implementing _alt_source in get_pids() * refactor: Removing debug code * refactor: Removing redundant code * refactor: Updating the default_predicate() * refactor: split str command line using shlex.split() instead of str.split() * tests: quoting tmux start command arg * refacotr: Enhance get_pids() performance * tests: excaping the backslash in the regex pattern * Adding debuging code * tests: Adding exact arg to get_process_pids() * Add debugging code * refactor: Adding delay arg to check_stop() and check_start() * tests: Disable match againest full cmdline when `exact` arg set to True * docs: Addding examples in the module docstring * docs: completing and formating docstrings * Removing some debug messages * refactor: Adding timeout arg to check_stop() and check_start() * refactor: Adding `sig` and `timeout` args to kill_user_processes() * feat: Adding new function kill_all_pids() * refactor:Now kill() return `None` when succeed * refactor:Now check_start() return `None` when succeed * refactor:Now check_stop() return `None` when succeed * tests: Adapting to Change in process.kill() return value * tests: Adapting to Change in process.kill() return value * refactor:Now kill_process_by_port() doesn't expect return value from kill() * refactor: Simplifying long ternary operator statement as suggested by @abom * fix: Removing misplaced and redundant try/except * refactor: Removing redundant debug message * refactor:kill_all() now deprecated and removed in favor of kill_process_by_name() * tests: Adapting and replacing kill_all() with kill_process_by_name() * fix:change default `die` arg value to False for get_process_object() * refactor: Updating set_env_var() remove redundant try/except statment * Docs: Updating docstring * Docs: Updating get_suer_processes() docstring * refactor: Raising proper exception type * fix: Skipping a test that require root privileges, if user is not root * refactor: apply a suggested change * more cleanup with @sameh-farouk * simplify get_pids predicate matching for processes Co-authored-by: Abdelrahman Ghanem <abom.jdev@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
js-ng/jumpscale/sals/process/__init__.py
Lines 134 to 135 in 4c8b7d9
the current implementation for the kill function in process sal makes it returns True immediately after sending the signal, without making sure that the process is killed. it returns a false positive in cases:
1- the Process (as usual) takes some time to terminate.
2- the Process waits for something or busy.
steps to reproduce:
1- open a new terminal window, execute
pscommand and make note of the bash PID2- execute
topcommand to make the bash busy3- from vscode terminal or other terminal import the process sal as process
4- execute
process.kill($PID)replace $PID with the PID taken from step 15- the function will return True stating that the process should be killed, but the shell process and its children process still operate normally.
to fix this:
we need to check and wait for the process to disappear after sending the signal, if still a live for a period of time then we raise an exception or returns False.
also, it is possible to enhance the function with a bool parameter making it send SIGKILL if SIGTERM failed to kill the process.
The text was updated successfully, but these errors were encountered: