refactor: lifecycle#39
Merged
Merged
Conversation
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
- move native symbol bindings into Entry tables - clear registered function vars after successful unload - update api-check for Entry-based registrations Co-authored-by: Copilot <copilot@github.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了 3 个问题，并给出了一些总体反馈：
clearFuncVar假定reflect.ValueOf(ptr)总是有效；如果传入的是nil，val.Kind()会触发 panic，所以在检查Kind之前，可以考虑先用val.IsValid()做保护。Initialize/Shutdown以及全局的loadedLibs切片目前没有做同步；如果这些 API 可能会被多个 goroutine 调用，你可能需要加一个互斥锁，或者通过文档/其他方式明确禁止并发使用。
给 AI 代理的提示
Please address the comments from this code review:
## Overall Comments
- `clearFuncVar` 假定 `reflect.ValueOf(ptr)` 总是有效；如果传入的是 `nil`，`val.Kind()` 会触发 panic，所以在检查 `Kind` 之前，可以考虑先用 `val.IsValid()` 做保护。
- `Initialize/Shutdown` 以及全局的 `loadedLibs` 切片目前没有做同步；如果这些 API 可能会被多个 goroutine 调用，你可能需要加一个互斥锁，或者通过文档/其他方式明确禁止并发使用。
## Individual Comments
### Comment 1
<location path="internal/native/native.go" line_range="23-27" />
<code_context>
+}
+
+var (
+ libraries = []Library{
+ {name: maaFrameworkName, init: initFramework, release: releaseFramework},
+ {name: maaToolkitName, init: initToolkit, release: releaseToolkit},
+ {name: maaAgentServerName, init: initAgentServer, release: releaseAgentServer},
+ {name: maaAgentClientName, init: initAgentClient, release: releaseAgentClient},
+ }
+ loadedLibs []Library
</code_context>
<issue_to_address>
**issue (bug_risk):** 跨文件的变量初始化顺序可能导致 `libraries` 切片中的库名为空
`libraries` 在包初始化阶段是由定义在其他文件中的 `maaFrameworkName`、`maaToolkitName`、`maaAgentServerName` 和 `maaAgentClientName` 这些变量构建出来的。由于 Go 不保证跨文件的初始化顺序，在构建 `libraries` 时，这些变量可能仍然是零值，从而导致 `Library.name` 为空，并破坏依赖名称的逻辑。为避免这种情况，可以考虑：把这些名称改为 `const`，或者在 `libraries` 中直接内联字符串字面量，或者在一个 `init` 函数中构造 `libraries`，并确保该函数在这些名称被赋值之后运行。
</issue_to_address>
### Comment 2
<location path="maa.go" line_range="146-147" />
<code_context>
func Init(opts ...InitOption) error {
if inited {
- return ErrAlreadyInitialized
+ return nil
}
</code_context>
<issue_to_address>
**issue (bug_risk):** 重复调用 Init 时静默空操作会改变公共 API，并可能掩盖误用
之前，第二次调用 `Init` 会返回 `ErrAlreadyInitialized`，从而让调用方能够检测并处理重复初始化。现在它会静默成功，这改变了 API 行为，并可能掩盖配置错误（例如误以为新的选项会生效）。如果 API 兼容性很重要，建议保留这个错误，或者在文档中清晰说明这一行为变更，并/或在忽略第二次初始化时记录日志。
</issue_to_address>
### Comment 3
<location path="internal/native/native.go" line_range="75-76" />
<code_context>
+
+ loadedLibs = failed
+
+ if len(errs) > 0 {
+ return fmt.Errorf("failed to release libraries: %w", errors.Join(errs...))
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Shutdown 的错误报告可以包含哪些库释放失败的信息
`Shutdown` 已经通过 `failed []Library` 追踪释放失败的库，但返回的错误只是简单地用一个通用消息包装合并后的错误，这会让调用方难以知道具体是哪几个库释放失败。既然已经有 `failed` 切片，可以考虑在更高层的错误消息中包含失败库的名称（例如 `lib.name`），以提升可调试性。
Suggested implementation:
```golang
import (
"errors"
"fmt"
"reflect"
"strings"
)
```
```golang
loadedLibs = failed
```
```golang
if len(errs) > 0 {
failedNames := make([]string, 0, len(failed))
for _, lib := range failed {
failedNames = append(failedNames, lib.name)
}
return fmt.Errorf(
"failed to release libraries (%s): %w",
strings.Join(failedNames, ", "),
errors.Join(errs...),
)
}
```
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续的评审。
Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- clearFuncVar assumes reflect.ValueOf(ptr) is always valid; if a nil is ever passed in, val.Kind() will panic, so consider guarding with val.IsValid() before checking Kind.
- Initialize/Shutdown and the global loadedLibs slice are not synchronized; if these APIs are ever called from multiple goroutines, you may want to add a mutex or otherwise document/guard against concurrent use.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- clearFuncVar assumes reflect.ValueOf(ptr) is always valid; if a nil is ever passed in, val.Kind() will panic, so consider guarding with val.IsValid() before checking Kind.
- Initialize/Shutdown and the global loadedLibs slice are not synchronized; if these APIs are ever called from multiple goroutines, you may want to add a mutex or otherwise document/guard against concurrent use.
## Individual Comments
### Comment 1
<location path="internal/native/native.go" line_range="23-27" />
<code_context>
+}
+
+var (
+ libraries = []Library{
+ {name: maaFrameworkName, init: initFramework, release: releaseFramework},
+ {name: maaToolkitName, init: initToolkit, release: releaseToolkit},
+ {name: maaAgentServerName, init: initAgentServer, release: releaseAgentServer},
+ {name: maaAgentClientName, init: initAgentClient, release: releaseAgentClient},
+ }
+ loadedLibs []Library
</code_context>
<issue_to_address>
**issue (bug_risk):** Cross-file var initialization order can lead to empty library names in `libraries` slice
`libraries` is built at package init time from `maaFrameworkName`, `maaToolkitName`, `maaAgentServerName`, and `maaAgentClientName` vars defined in other files. Since Go does not guarantee cross-file init order, these may still be zero-valued when `libraries` is constructed, giving empty `Library.name`s and breaking name-dependent logic. To avoid this, either make these names `const`, inline the string literals in `libraries`, or construct `libraries` in an init function that runs after the names are set.
</issue_to_address>
### Comment 2
<location path="maa.go" line_range="146-147" />
<code_context>
func Init(opts ...InitOption) error {
if inited {
- return ErrAlreadyInitialized
+ return nil
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Silent no-op on repeated Init calls changes the public API and may hide misuse
Previously, a second `Init` call returned `ErrAlreadyInitialized`, allowing callers to detect and handle double init. Now it silently succeeds, which changes the API and can hide config mistakes (e.g., expecting new options to apply). If API compatibility matters, consider keeping the error or clearly documenting this behavioral change, and/or logging when a second init is ignored.
</issue_to_address>
### Comment 3
<location path="internal/native/native.go" line_range="75-76" />
<code_context>
+
+ loadedLibs = failed
+
+ if len(errs) > 0 {
+ return fmt.Errorf("failed to release libraries: %w", errors.Join(errs...))
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Shutdown error reporting could include which libraries failed to release
`Shutdown` already tracks `failed []Library`, but the returned error just wraps the joined errors with a generic message, which makes it hard to see which libraries failed. Since you have the `failed` slice, consider including the failing library names (e.g. `lib.name`) in a higher-level error message to improve debuggability.
Suggested implementation:
```golang
import (
"errors"
"fmt"
"reflect"
"strings"
)
```
```golang
loadedLibs = failed
```
```golang
if len(errs) > 0 {
failedNames := make([]string, 0, len(failed))
for _, lib := range failed {
failedNames = append(failedNames, lib.name)
}
return fmt.Errorf(
"failed to release libraries (%s): %w",
strings.Join(failedNames, ", "),
errors.Join(errs...),
)
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+75
to
+76
| if len(errs) > 0 { | ||
| return fmt.Errorf("failed to release libraries: %w", errors.Join(errs...)) |
Contributor
There was a problem hiding this comment.
suggestion: Shutdown 的错误报告可以包含哪些库释放失败的信息
Shutdown 已经通过 failed []Library 追踪释放失败的库，但返回的错误只是简单地用一个通用消息包装合并后的错误，这会让调用方难以知道具体是哪几个库释放失败。既然已经有 failed 切片，可以考虑在更高层的错误消息中包含失败库的名称（例如 lib.name），以提升可调试性。
Suggested implementation:
import (
"errors"
"fmt"
"reflect"
"strings"
) loadedLibs = failed if len(errs) > 0 {
failedNames := make([]string, 0, len(failed))
for _, lib := range failed {
failedNames = append(failedNames, lib.name)
}
return fmt.Errorf(
"failed to release libraries (%s): %w",
strings.Join(failedNames, ", "),
errors.Join(errs...),
)
}Original comment in English
suggestion: Shutdown error reporting could include which libraries failed to release
Shutdown already tracks failed []Library, but the returned error just wraps the joined errors with a generic message, which makes it hard to see which libraries failed. Since you have the failed slice, consider including the failing library names (e.g. lib.name) in a higher-level error message to improve debuggability.
Suggested implementation:
import (
"errors"
"fmt"
"reflect"
"strings"
) loadedLibs = failed if len(errs) > 0 {
failedNames := make([]string, 0, len(failed))
for _, lib := range failed {
failedNames = append(failedNames, lib.name)
}
return fmt.Errorf(
"failed to release libraries (%s): %w",
strings.Join(failedNames, ", "),
errors.Join(errs...),
)
}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors native library lifecycle management by introducing centralized Library/Entry registries in internal/native, improving init/shutdown error handling, and updating the tools/api-check parser/docs to validate registrations from []Entry tables.
Changes:
- Add
Libraryregistry +Initialize/Shutdownorchestration that tracks successfully loaded native libraries and releases them in reverse order with aggregated errors. - Replace ad-hoc
purego.RegisterLibFunccall lists with package-level[]Entrytables and centralized unregister/clear behavior. - Update
tools/api-checkto parse[]Entryregistrations and add coverage tests + documentation updates.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/api-check/internal/checker/native_check_test.go | Adds a unit test ensuring Go registration parsing works via []Entry tables and reports entry mismatches. |
| tools/api-check/internal/checker/native_check.go | Refactors registration parsing to extract symbols from []Entry composite literals and report mismatches with locations. |
| tools/api-check/README.md | Updates documentation to reflect []Entry-table based registrations and new mismatch terminology. |
| maa.go | Switches to native.Initialize/native.Shutdown; makes Init idempotent and simplifies Release behavior. |
| internal/native/native.go | Introduces Library/Entry abstractions, loaded-library tracking, aggregated shutdown errors, and func-var clearing helper. |
| internal/native/framework.go | Centralizes framework registrations into frameworkEntries and implements releaseFramework/unregisterFramework. |
| internal/native/toolkit.go | Centralizes toolkit registrations into toolkitEntries and implements releaseToolkit/unregisterToolkit. |
| internal/native/agent_server.go | Centralizes agent-server registrations into agentServerEntries and implements releaseAgentServer/unregisterAgentServer. |
| internal/native/agent_client.go | Centralizes agent-client registrations into agentClientEntries and implements releaseAgentClient/unregisterAgentClient. |
| internal/buffer/main_test.go | Updates test initialization to call native.Initialize. |
Comments suppressed due to low confidence (1)
maa.go:22
- This changes the public API surface: the exported sentinel errors
ErrAlreadyInitialized/ErrNotInitializedwere removed. If downstream callers relied on these symbols (or on comparing errors), this is a breaking change. Consider keeping the exported vars (possibly deprecated) or documenting the removal and bumping the major version accordingly.
var (
inited bool
ErrSetLogDir = errors.New("failed to set log directory")
ErrSetSaveDraw = errors.New("failed to set save draw option")
ErrSetStdoutLevel = errors.New("failed to set stdout level")
ErrSetDebugMode = errors.New("failed to set debug mode")
ErrSetSaveOnError = errors.New("failed to set save on error option")
ErrSetDrawQuality = errors.New("failed to set draw quality")
ErrSetRecoImageCacheLimit = errors.New("failed to set recognition image cache limit")
ErrLoadPlugin = errors.New("failed to load plugin")
ErrEmptyLogDir = errors.New("log directory path is empty")
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <copilot@github.com>
…ctions Co-authored-by: Copilot <copilot@github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
通过引入共享库元数据、集中化的入口表以及更健壮的初始化/关闭流程来完善原生库的生命周期管理，并更新 API 检查工具和测试，使其适应新的注册模式。
Enhancements:
[]Entry表中，并从这些表派生 Maa* 名称常量，从而减少重复并与直接调用RegisterLibFunc解耦。clearFuncVar辅助函数在库卸载时将函数变量清零，避免关闭后残留的失效绑定。maa.Init/Release语义，以支持幂等初始化，并在无需事先检查初始化状态的情况下始终尝试执行关闭。Tests:
[]Entry的注册方式，并添加单元测试以验证新的注册发现机制和不匹配报告。native.Initialize入口点来替代已移除的Init函数。Original summary in English
Summary by Sourcery
Refine native library lifecycle management by introducing shared library metadata, centralized entry tables, and robust initialization/shutdown, while updating API-check tooling and tests to work with the new registration pattern.
Enhancements:
Tests:
增强内容：
Original summary in English
Summary by Sourcery
通过引入共享库元数据、集中化的入口表以及更健壮的初始化/关闭流程来完善原生库的生命周期管理，并更新 API 检查工具和测试，使其适应新的注册模式。
Enhancements:
[]Entry表中，并从这些表派生 Maa* 名称常量，从而减少重复并与直接调用RegisterLibFunc解耦。clearFuncVar辅助函数在库卸载时将函数变量清零，避免关闭后残留的失效绑定。maa.Init/Release语义，以支持幂等初始化，并在无需事先检查初始化状态的情况下始终尝试执行关闭。Tests:
[]Entry的注册方式，并添加单元测试以验证新的注册发现机制和不匹配报告。native.Initialize入口点来替代已移除的Init函数。Original summary in English
Summary by Sourcery
Refine native library lifecycle management by introducing shared library metadata, centralized entry tables, and robust initialization/shutdown, while updating API-check tooling and tests to work with the new registration pattern.
Enhancements:
Tests: