Skip to content

改进 mxsml 动态库加载错误上下文#15

Open
ghangz wants to merge 3 commits into
MetaX-MACA:mainfrom
ghangz:mengz/mxsml-load-error-context
Open

改进 mxsml 动态库加载错误上下文#15
ghangz wants to merge 3 commits into
MetaX-MACA:mainfrom
ghangz:mengz/mxsml-load-error-context

Conversation

@ghangz

@ghangz ghangz commented Jun 25, 2026

Copy link
Copy Markdown

这个改动增强了 mxsml 动态库加载失败时的诊断信息,保留了原始错误链路,并把实际尝试过的路径一并带出来,方便在安装路径不一致或部署包缺失时快速定位问题。

相关单元测试已经补充并通过,重点覆盖了错误解包和候选路径回显行为,本地测试结果稳定。

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request improves error reporting when loading libmxsml.so fails by introducing a custom LoadError struct that captures the attempted search paths and the underlying cause, along with corresponding unit tests. The reviewer suggests implementing the Unwrap() error method on LoadError to support Go's error wrapping standards (with a corresponding unit test), passing the full list of attemptedPaths when dlopen fails for consistency, and addresses a potential thread-safety issue with retrieving dlerror after unlocking the OS thread.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread pkg/lib/load_error.go
Comment on lines +13 to +22
func (e *LoadError) Error() string {
parts := []string{"failed to load libmxsml.so"}
if len(e.AttemptedPaths) > 0 {
parts = append(parts, fmt.Sprintf("attempted paths: %s", strings.Join(e.AttemptedPaths, ", ")))
}
if e.Cause != nil {
parts = append(parts, fmt.Sprintf("cause: %v", e.Cause))
}
return strings.Join(parts, "; ")
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

为了更好地支持 Go 的错误包装(Error Wrapping)规范,建议为 LoadError 结构体实现 Unwrap() error 方法。这样上游调用者可以使用 errors.Iserrors.As 来获取和判断底层的 Cause 错误。

func (e *LoadError) Error() string {
	parts := []string{"failed to load libmxsml.so"}
	if len(e.AttemptedPaths) > 0 {
		parts = append(parts, fmt.Sprintf("attempted paths: %s", strings.Join(e.AttemptedPaths, ", ")))
	}
	if e.Cause != nil {
		parts = append(parts, fmt.Sprintf("cause: %v", e.Cause))
	}
	return strings.Join(parts, "; ")
}

func (e *LoadError) Unwrap() error {
	return e.Cause
}

Comment on lines +20 to +27
func TestLoadErrorCopiesAttemptedPaths(t *testing.T) {
paths := []string{"/opt/maca/lib/libmxsml.so"}
err := newLoadError(paths, nil).(*LoadError)
paths[0] = "/mutated"
if err.AttemptedPaths[0] != "/opt/maca/lib/libmxsml.so" {
t.Fatalf("expected attempted paths to be copied, got %v", err.AttemptedPaths)
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

为了确保新实现的 Unwrap 方法能够正常工作,建议在测试文件中补充对应的单元测试。

Suggested change
func TestLoadErrorCopiesAttemptedPaths(t *testing.T) {
paths := []string{"/opt/maca/lib/libmxsml.so"}
err := newLoadError(paths, nil).(*LoadError)
paths[0] = "/mutated"
if err.AttemptedPaths[0] != "/opt/maca/lib/libmxsml.so" {
t.Fatalf("expected attempted paths to be copied, got %v", err.AttemptedPaths)
}
}
func TestLoadErrorCopiesAttemptedPaths(t *testing.T) {
paths := []string{"/opt/maca/lib/libmxsml.so"}
err := newLoadError(paths, nil).(*LoadError)
paths[0] = "/mutated"
if err.AttemptedPaths[0] != "/opt/maca/lib/libmxsml.so" {
t.Fatalf("expected attempted paths to be copied, got %v", err.AttemptedPaths)
}
}
func TestLoadErrorUnwrap(t *testing.T) {
cause := errors.New("dlopen failed")
err := newLoadError(nil, cause)
if !errors.Is(err, cause) {
t.Fatalf("expected wrapped error to be unwrappable, got %v", err)
}
}

Comment thread pkg/lib/lib.go Outdated
handle = C.dlopen(libPath, C.int(g_mxsmlLib.flag))
if handle == nil {
return getDlError()
return newLoadError([]string{installPath}, getDlError())

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

dlopen(installPath) 失败时,建议将 attemptedPaths(包含所有候选路径)传递给 newLoadError,而不是仅传递 []string{installPath}。这样可以保持与第 73 行错误处理的一致性,让用户在排查时能够清晰地看到完整的候选路径列表,更符合 PR 描述中“把候选路径和底层错误一起带出来”的初衷。

Suggested change
return newLoadError([]string{installPath}, getDlError())
return newLoadError(attemptedPaths, getDlError())

Comment thread pkg/lib/lib.go
var installPath string
// check lib path
libPathList := []string{
attemptedPaths := []string{

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

在第 49 行,代码尝试了第一次 C.dlopen(libName)。如果它失败了,由于在第 50 行立即调用了 runtime.UnlockOSThread(),此时当前 Goroutine 可能会被调度到其他 OS 线程。因此,后续如果想获取第一次 dlopen 的底层错误(通过 dlerror),将会因为线程切换而无法准确获取(dlerror 是线程局部的)。

虽然这超出了本次 diff 的直接修改范围,但建议在后续重构中,将第一次 dlopen 的错误获取也放入 LockOSThread 保护区间内,并将其作为 LoadErrorCause 之一返回,避免在系统路径中存在库但因依赖缺失导致加载失败时,错误被误判为“候选路径不存在”。

@ghangz ghangz changed the title 改进 mxsml 动态库加载错误上下文 ?? mxsml ?????????? Jun 26, 2026
@ghangz ghangz changed the title ?? mxsml ?????????? 改进 mxsml 动态库加载错误上下文 Jun 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant