Skip to content

Feat: Add the PagePool.TryGet method while it returns an error for processing. #1051

Merged
ysmood merged 7 commits intogo-rod:mainfrom
zeoxisca:main
May 6, 2024
Merged

Feat: Add the PagePool.TryGet method while it returns an error for processing. #1051
ysmood merged 7 commits intogo-rod:mainfrom
zeoxisca:main

Conversation

@zeoxisca
Copy link
Copy Markdown
Contributor

@zeoxisca zeoxisca commented Apr 30, 2024

Development guide

Link

Test on local before making the PR

go run ./lib/utils/simple-check

PagePool Change

  • PagePool/BrowserPool Add the MustGet method to replace Get, and Get returns an error for processing
  • Modify CleanUp method to prevent stucking

The PagePool.Get can now only accept create func() *Page as input, so there is no way to output the error that may exist in the create func other than using a closure or using recover, and it is inconvenient for users in a multi-routine environment to perform logical processing based on the actual error that occurred. I added the PagePool.MustGet method to solve this. However, I'm not sure whether this change would be in line with the original design.

Besides, PagePool.CleanUp may stuck while pp is not full (forget/panic to Put, etc). I also think this might be an issue when using PagePool. Use select to prevent stucking

Copy link
Copy Markdown
Member

@ysmood ysmood left a comment

Choose a reason for hiding this comment

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

👍🏼

Comment thread utils.go Outdated
}

// TryGet a page from the pool, allow error. Use the [PagePool.Put] to make it reusable later.
func (pp PagePool) TryGet(create func() (*Page, error)) (*Page, error) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
func (pp PagePool) TryGet(create func() (*Page, error)) (*Page, error) {
func (pp PagePool) Get(create func() (*Page, error)) (*Page, error) {

Could you help to rename the Get to MustGet? Same for the browser pool.

Copy link
Copy Markdown
Contributor Author

@zeoxisca zeoxisca May 5, 2024

Choose a reason for hiding this comment

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

I've tried it before, but MustGet is restricted to be implemented in must.go because of the lint.
Changing to PagePool.Get also could involve the chrome-extensions. I'll rename it if these changes don't impact a lot.

Copy link
Copy Markdown
Member

@ysmood ysmood May 5, 2024

Choose a reason for hiding this comment

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

Sure, move the MustGet to must.go file. Keep your Get one in the utils.go file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown
Member

@ysmood ysmood left a comment

Choose a reason for hiding this comment

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

LGTM!

@ysmood ysmood merged commit 2d3e61f into go-rod:main May 6, 2024
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.

2 participants