Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Close pool in Cleanup to prevent Get getting stuck in concurrent scenarios. #1117

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions browser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,27 @@ func TestBrowserPool(t *testing.T) {
})
}

func TestBrowserPoolNotStuck(t *testing.T) {
g := got.T(t)

pool := rod.NewBrowserPool(3)

pool.Cleanup(func(p *rod.Browser) {
p.MustClose()
})

b, err := pool.Get(func() (*rod.Browser, error) {
browser := rod.New()
return browser, browser.Connect()
})
if err != nil {
g.Equal(err, errors.New("pool has been cleaned up"))
} else {
pool.Put(b) // will panic
g.Fail()
}
}

func TestOldBrowser(t *testing.T) {
t.Skip()

Expand Down
19 changes: 19 additions & 0 deletions page_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -992,6 +992,25 @@ func TestPagePool(t *testing.T) {
})
}

func TestPagePoolNotStuck(t *testing.T) {
g := setup(t)
pool := rod.NewPagePool(3)

pool.Cleanup(func(p *rod.Page) {
p.MustClose()
})

b, err := pool.Get(func() (*rod.Page, error) {
return g.browser.Page(proto.TargetCreateTarget{})
})
if err != nil {
g.Equal(err, errors.New("pool has been cleaned up"))
} else {
pool.Put(b) // will panic
g.Fail()
}
}

func TestPageUseNonExistSession(t *testing.T) {
g := setup(t)

Expand Down
19 changes: 13 additions & 6 deletions utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"context"
"encoding/base64"
"encoding/json"
"errors"
"fmt"
"io"
"log"
Expand Down Expand Up @@ -102,16 +103,20 @@
return p
}

// Get a elem from the pool, allow error. Use the [Pool[T].Put] to make it reusable later.
// Get an elem from the pool, allow error. Use the [Pool[T].Put] to make it reusable later.
func (p Pool[T]) Get(create func() (*T, error)) (elem *T, err error) {
elem = <-p
if elem == nil {
elem, err = create()
// use range to prevent getting stuck in concurrent scenarios
for elem = range p {
if elem == nil {
elem, err = create()
}
return

Check failure on line 113 in utils.go

View workflow job for this annotation

GitHub Actions / test-linux

SA4004: the surrounding loop is unconditionally terminated (staticcheck)
}
return
// here p has been closed
return nil, errors.New("pool has been cleaned up")
}

// Put an elem back to the pool.
// Put an elem back to the pool. Should not be called when Get returns Error
func (p Pool[T]) Put(elem *T) {
p <- elem
}
Expand All @@ -127,6 +132,8 @@
default:
}
}
// close Pool after iterating to prevent Get getting stuck
close(p)
}

var _ io.ReadCloser = &StreamReader{}
Expand Down
Loading