fix: clean up duplicate platform adapters on reload#6102
fix: clean up duplicate platform adapters on reload#6102stablegenius49 wants to merge 1 commit intoAstrBotDevs:masterfrom
Conversation
dosubot
bot
added
the
size:L
Summary of ChangesHello, 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 platform management system to prevent issues arising from duplicate or orphaned platform instances. It ensures that when a platform is reloaded, any existing instance with the same ID is properly terminated first. Additionally, the termination process has been improved to proactively identify and shut down any untracked, "orphaned" instances that might still be running for a given platform ID, leading to a more robust and predictable system state. Highlights
Changelog
Activity
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
|
dosubot
bot
added
the
area:platform
![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 effectively addresses the issue of duplicate platform adapters on reload. The changes in load_platform to terminate existing instances and the improved cleanup logic in terminate_platform are well-implemented. The addition of regression tests is also a great improvement to ensure this behavior is maintained. I have one suggestion to refactor the terminate_platform method for better performance and clarity.
| tracked_inst: Platform | None = None | ||
| info = self._inst_map.pop(platform_id, None) | ||
| if info: | ||
| tracked_inst = info["inst"] | ||
|
|
||
| # client_id = self._inst_map.pop(platform_id, None) | ||
| info = self._inst_map.pop(platform_id) | ||
| client_id = info["client_id"] | ||
| inst: Platform = info["inst"] | ||
| try: | ||
| self.platform_insts.remove( | ||
| next( | ||
| inst | ||
| for inst in self.platform_insts | ||
| if inst.client_self_id == client_id | ||
| ), | ||
| ) | ||
| except Exception: | ||
| logger.warning(f"可能未完全移除 {platform_id} 平台适配器") | ||
| insts_to_terminate: list[Platform] = [] | ||
| if tracked_inst is not None: | ||
| insts_to_terminate.append(tracked_inst) | ||
|
|
||
| for inst in list(self.platform_insts): | ||
| if inst in insts_to_terminate: | ||
| continue | ||
| if getattr(inst, "config", {}).get("id") == platform_id: | ||
| insts_to_terminate.append(inst) | ||
|
|
||
| if not insts_to_terminate: | ||
| return | ||
|
|
||
| logger.info(f"正在尝试终止 {platform_id} 平台适配器 ...") | ||
|
|
||
| for inst in insts_to_terminate: | ||
| while inst in self.platform_insts: | ||
| self.platform_insts.remove(inst) | ||
| await self._terminate_inst_and_tasks(inst) |
There was a problem hiding this comment.
The terminate_platform method can be refactored for better performance and readability.
- Efficient Instance Collection: Using a set comprehension to collect all relevant instances is more efficient than the current loop with list membership checks.
- Efficient Instance Removal: A list comprehension is the idiomatic and performant way to filter
self.platform_insts, rather than usingremove()in a loop. - Concurrent Termination: Terminating instances concurrently with
asyncio.gatherwill improve performance, as these are independent I/O-bound operations.
Applying these changes will make the function more robust and faster.
info = self._inst_map.pop(platform_id, None)
tracked_inst = info["inst"] if info else None
insts_to_terminate = {
inst
for inst in self.platform_insts
if getattr(inst, "config", {}).get("id") == platform_id
}
if tracked_inst:
insts_to_terminate.add(tracked_inst)
if not insts_to_terminate:
return
logger.info(f"正在尝试终止 {platform_id} 平台适配器 ...")
self.platform_insts = [
inst for inst in self.platform_insts if inst not in insts_to_terminate
]
await asyncio.gather(
*(self._terminate_inst_and_tasks(inst) for inst in insts_to_terminate)
)2 participants
Summary
terminate_platform()clean up orphaned instances that still share the same config IDTesting
/opt/homebrew/opt/python@3.11/bin/python3.11 -m ruff format astrbot/core/platform/manager.py tests/unit/test_platform_manager.py/opt/homebrew/opt/python@3.11/bin/python3.11 -m ruff check astrbot/core/platform/manager.py tests/unit/test_platform_manager.py/opt/homebrew/opt/python@3.11/bin/python3.11 -m pytest -q tests/unit/test_platform_manager.pyFixes #6100.