enh(taosd): improve process termination logic on Windows #33624
Conversation
Summary of ChangesHello @tomchon, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the process cleanup mechanism within the "destroy" method of "taosd.py" for Windows environments. The primary goal is to bolster the reliability and efficiency of terminating "taosd"-related processes by implementing more comprehensive process identification and transitioning to a batch termination approach, thereby streamlining resource management during testing. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the process cleanup logic for Windows by finding all relevant processes and terminating them in a batch, which is more robust and efficient. The changes look good, but there's a small area for improvement in the implementation. I've added a comment to refactor the process killing logic to avoid fetching process information multiple times, which will make it even more efficient.
| pids_to_kill = [] | ||
| for proc in psutil.process_iter(['pid', 'name', 'cmdline']): | ||
| if ('mintty' in proc.info['name'] | ||
| and proc.info['cmdline'] # 确保 cmdline 非空 | ||
| and any('taosd' in arg for arg in proc.info['cmdline']) | ||
| ): | ||
| self.logger.debug(proc.info) | ||
| self.logger.debug("Found taosd.exe process with PID: %s", proc.info['pid']) | ||
| pid = proc.info['pid'] | ||
| #kernel32 = ctypes.windll.kernel32 | ||
| #kernel32.GenerateConsoleCtrlEvent(0, pid) | ||
| killCmd = f"taskkill /PID {pid} /T /F" | ||
| #killCmd = "for /f %%a in ('wmic process where \"name='taosd.exe'\" get processId ^| xargs echo ^| awk ^'{print $2}^' ^&^& echo aa') do @(ps | grep %%a | awk '{print $1}' | xargs)" | ||
| self._remote.cmd_windows(fqdn, [killCmd]) | ||
|
|
||
| try: | ||
| pname = (proc.info.get('name') or '').lower() | ||
| pcmd = proc.info.get('cmdline') or [] | ||
| # 匹配 taosd 或 tmq_sim | ||
| if (('mintty' in pname and any('taosd' in str(a).lower() for a in pcmd)) | ||
| or 'taosd' in pname | ||
| or 'tmq_sim' in pname | ||
| or any('tmq_sim' in str(a).lower() for a in pcmd)): | ||
| self.logger.info(f"Found process {pname} (PID: {proc.info['pid']})") | ||
| pids_to_kill.append(proc.info['pid']) | ||
| except (psutil.NoSuchProcess, psutil.AccessDenied): | ||
| continue | ||
|
|
||
| # 批量杀进程（减少远程调用） | ||
| if pids_to_kill: | ||
| # kernel32 = ctypes.windll.kernel32 | ||
| # kernel32.GenerateConsoleCtrlEvent(0, pid) | ||
| #killCmd = "for /f %%a in ('wmic process where \"name='taosd.exe'\" get processId ^| xargs echo ^| awk ^'{print $2}^' ^&^& echo aa') do @(ps | grep %%a | awk '{print $1}' | xargs)" | ||
| for pid in pids_to_kill: | ||
| try: | ||
| proc = psutil.Process(pid) | ||
| proc_name = proc.name() | ||
| self.logger.info(f"Killing process {proc_name} (PID: {pid})") | ||
| except (psutil.NoSuchProcess, psutil.AccessDenied): | ||
| self.logger.warning(f"Cannot access process info for PID: {pid}") | ||
|
|
||
| kill_cmds = [f"taskkill /PID {pid} /T /F" for pid in pids_to_kill] | ||
| self._remote.cmd_windows(fqdn, kill_cmds) | ||
| self.logger.info(f"Killed {len(pids_to_kill)} processes on {fqdn}") |
There was a problem hiding this comment.
The logic to find and kill processes has been improved. However, there's a small inefficiency. You are iterating through the processes to get their PIDs, and then in a separate loop, you are creating psutil.Process(pid) objects again just for logging. This is redundant and inefficient as it involves extra system calls.
You can make this more efficient by storing the process name along with the PID in the first loop, and then using that stored information for logging. This avoids re-fetching process information.
Here is a suggested refactoring that combines these steps and improves efficiency:
pids_to_kill = []
proc_name_map = {}
for proc in psutil.process_iter(['pid', 'name', 'cmdline']):
try:
proc_name = proc.info.get('name') or ''
pname = proc_name.lower()
pcmd = proc.info.get('cmdline') or []
# 匹配 taosd 或 tmq_sim
if (('mintty' in pname and any('taosd' in str(a).lower() for a in pcmd))
or 'taosd' in pname
or 'tmq_sim' in pname
or any('tmq_sim' in str(a).lower() for a in pcmd)):
pid = proc.info['pid']
self.logger.info(f"Found process {proc_name} (PID: {pid})")
pids_to_kill.append(pid)
proc_name_map[pid] = proc_name
except (psutil.NoSuchProcess, psutil.AccessDenied):
continue
# 批量杀进程（减少远程调用）
if pids_to_kill:
# kernel32 = ctypes.windll.kernel32
# kernel32.GenerateConsoleCtrlEvent(0, pid)
#killCmd = "for /f %%a in ('wmic process where \"name='taosd.exe'\" get processId ^| xargs echo ^| awk ^'{print $2}^' ^&^& echo aa') do @(ps | grep %%a | awk '{print $1}' | xargs)"
for pid in pids_to_kill:
self.logger.info(f"Killing process {proc_name_map.get(pid, 'unknown')} (PID: {pid})")
kill_cmds = [f"taskkill /PID {pid} /T /F" for pid in pids_to_kill]
self._remote.cmd_windows(fqdn, kill_cmds)
self.logger.info(f"Killed {len(pids_to_kill)} processes on {fqdn}")There was a problem hiding this comment.
Pull Request Overview
This pull request enhances the Windows process termination logic in the destroy method of taosd.py. The changes aim to improve how taosd-related processes are identified and terminated on Windows systems.
Key Changes:
- Expanded process matching to include both
taosdandtmq_simprocesses, with improved detection logic for processes running underminttyshells - Refactored from single-process termination to batch collection and termination of all matching processes
- Enhanced error handling with try-except blocks around process iteration to handle inaccessible processes gracefully
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except (psutil.NoSuchProcess, psutil.AccessDenied): | ||
| continue | ||
|
|
||
| # 批量杀进程（减少远程调用） |
There was a problem hiding this comment.
[nitpick] Consider translating this comment to English for better maintainability and consistency with international collaboration. Suggested: # Batch kill processes (reduce remote calls)
| # 批量杀进程（减少远程调用） | |
| # Batch kill processes (reduce remote calls) |
| for proc in psutil.process_iter(['pid', 'name', 'cmdline']): | ||
| if ('mintty' in proc.info['name'] | ||
| and proc.info['cmdline'] # 确保 cmdline 非空 | ||
| and any('taosd' in arg for arg in proc.info['cmdline']) | ||
| ): | ||
| self.logger.debug(proc.info) | ||
| self.logger.debug("Found taosd.exe process with PID: %s", proc.info['pid']) | ||
| pid = proc.info['pid'] | ||
| #kernel32 = ctypes.windll.kernel32 | ||
| #kernel32.GenerateConsoleCtrlEvent(0, pid) | ||
| killCmd = f"taskkill /PID {pid} /T /F" | ||
| #killCmd = "for /f %%a in ('wmic process where \"name='taosd.exe'\" get processId ^| xargs echo ^| awk ^'{print $2}^' ^&^& echo aa') do @(ps | grep %%a | awk '{print $1}' | xargs)" | ||
| self._remote.cmd_windows(fqdn, [killCmd]) | ||
|
|
||
| try: | ||
| pname = (proc.info.get('name') or '').lower() | ||
| pcmd = proc.info.get('cmdline') or [] | ||
| # 匹配 taosd 或 tmq_sim | ||
| if (('mintty' in pname and any('taosd' in str(a).lower() for a in pcmd)) | ||
| or 'taosd' in pname | ||
| or 'tmq_sim' in pname | ||
| or any('tmq_sim' in str(a).lower() for a in pcmd)): | ||
| self.logger.info(f"Found process {pname} (PID: {proc.info['pid']})") | ||
| pids_to_kill.append(proc.info['pid']) | ||
| except (psutil.NoSuchProcess, psutil.AccessDenied): | ||
| continue |
There was a problem hiding this comment.
Critical bug: psutil.process_iter() collects process IDs from the local machine, but these PIDs are then used to kill processes on fqdn (line 379), which may be a remote machine. This causes the wrong processes to be killed when fqdn is not localhost.
For remote Windows hosts, you need to either:
- Use
self._remote.cmd_windows(fqdn, ...)to list processes on the remote machine first, then parse the output to get PIDs - Use a remote process enumeration method via WinRM
For localhost, the current approach works because cmd_windows detects localhost and executes locally.
This pull request improves the process cleanup logic for Windows in the
destroymethod oftaosd.py. The main change is to enhance how taosd-related processes are identified and terminated, making the code more robust and efficient.Process cleanup improvements:
taosdbut alsotmq_sim, and improved matching logic to handle variations in process names and command lines.