Skip to content

fix: close exit channel in Launch method to prevent goroutine leaks#1221

Open
excitedplus1s wants to merge 1 commit into
go-rod:mainfrom
excitedplus1s:fix/launch_goroutine_leak
Open

fix: close exit channel in Launch method to prevent goroutine leaks#1221
excitedplus1s wants to merge 1 commit into
go-rod:mainfrom
excitedplus1s:fix/launch_goroutine_leak

Conversation

@excitedplus1s

Copy link
Copy Markdown

Description

Upon execution, [Launcher.Launch] attaches to an existing session if a browser instance is already listening on the target port. Under this condition, with Leakless disabled (as it triggers antivirus false positives on Windows), the attaching process will leak goroutines and hang after executing [Launcher.Kill] and then [Launcher.Cleanup].

[Launcher.Launch] 在执行时,如果监听相关端口的浏览器实例已经启动,会采用附加的方式去接管会话。在这种状态下,如果未开启 Leakless(因为 Leakless 在Windows环境下会报毒),接管该浏览器实例的进程执行 [Launcher.Kill] 后,再去执行 [Launcher.Cleanup],会产生 goroutine 泄露导致进程被挂起。

Development guide

Link

Test on local before making the PR

go test ./lib/launcher/launcher_test.go -run TestLunchKill_GoroutineLeak

I've written two test cases for the goroutine leak scenarios.

Shared setup:
A launches a browser. B launches with the same [Launcher.RemoteDebuggingPort] and [Launcher.UserDataDir], attaching to A's session. Both have Leakless disabled.

Case 1: Both A and B perform [Launcher.Kill] and [Launcher.Cleanup].

Case 2: B performs [Launcher.Kill] and [Launcher.Cleanup]; A only handles the initial launch.

我为 goroutine 泄露的场景编写了两个测试用例。

共同设置:
A 启动一个浏览器实例。B 使用相同的 [Launcher.RemoteDebuggingPort] 和 [Launcher.UserDataDir] 启动,从而附加到 A 的会话上。两者的 Leakless 均被禁用。

**情况1:**A 和 B 均执行 [Launcher.Kill] 和 [Launcher.Cleanup]。

**情况2:**B 执行 [Launcher.Kill] 和 [Launcher.Cleanup],而 A 仅负责初始启动。

Disscusion

Perhaps we should extract the logic for reusing browser instances from [Launcher.Launch]. We should then clearly communicate to the caller that the browser instance is attachable, but that the [Launcher.Launch] method will not perform the attachment itself. Subsequently, we could provide a dedicated [Launcher.Attach] method to offer the functionality for reusing instances. This would give callers more flexibility and convenience in choosing and implementing various instance launch and release operations.

If you think this is a good idea, I can submit another PR based on this approach.

或许我们应该将reuse浏览器实例的操作从 [Launcher.Launch] 中移出,并明确的告知调用方这个浏览器实例可以被附加,但不执行附加操作。然后提供 [Launcher.Attach] 来提供重用实例的功能,这样对调用者来说,可以更方便的选择实现多种实例启动和释放操作。如果您觉得这个主意可行,我可以按照这个思路再提交一个PR。

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