rework CancelError into Cancel

That way, cancelling a context while checking the error is much simpler.
The context data already holds onto the cancel func, so this requires no
internal changes.

This renders the Browser.Shutdown API obsolete, even though it doesn't
do exactly the same. If we want Cancel to do a proper shutdown action
before cancelling a browser context and killing the process, we could do
that change within the cancel logic.
This commit is contained in:
Daniel Martí 2019-04-09 13:03:00 +02:00
parent e8122e4a26
commit 46982a1cac
4 changed files with 15 additions and 34 deletions

View File

@ -7,8 +7,6 @@ import (
"os" "os"
"sync/atomic" "sync/atomic"
"github.com/mailru/easyjson"
"github.com/chromedp/cdproto" "github.com/chromedp/cdproto"
"github.com/chromedp/cdproto/cdp" "github.com/chromedp/cdproto/cdp"
"github.com/chromedp/cdproto/runtime" "github.com/chromedp/cdproto/runtime"
@ -87,27 +85,6 @@ func NewBrowser(ctx context.Context, urlstr string, opts ...BrowserOption) (*Bro
return b, nil return b, nil
} }
// Shutdown shuts down the browser.
func (b *Browser) Shutdown() error {
if b.conn != nil {
if err := b.send(cdproto.CommandBrowserClose, nil); err != nil {
b.errf("could not close browser: %v", err)
}
return b.conn.Close()
}
return nil
}
// send writes the supplied message and params.
func (b *Browser) send(method cdproto.MethodType, params easyjson.RawMessage) error {
msg := &cdproto.Message{
ID: atomic.AddInt64(&b.next, 1),
Method: method,
Params: params,
}
return b.conn.Write(msg)
}
func (b *Browser) newExecutorForTarget(ctx context.Context, targetID target.ID, sessionID target.SessionID) *Target { func (b *Browser) newExecutorForTarget(ctx context.Context, targetID target.ID, sessionID target.SessionID) *Target {
if targetID == "" { if targetID == "" {
panic("empty target ID") panic("empty target ID")

View File

@ -133,11 +133,19 @@ func FromContext(ctx context.Context) *Context {
return c return c
} }
func CancelError(ctx context.Context) error { // Cancel cancels a chromedp context, waits for its resources to be cleaned up,
// and returns any error encountered during that process.
//
// Usually a "defer cancel()" will be enough for most use cases. This API is
// useful if you want to catch underlying cancel errors, such as when a
// temporary directory cannot be deleted.
func Cancel(ctx context.Context) error {
c := FromContext(ctx) c := FromContext(ctx)
if c == nil { if c == nil {
return ErrInvalidContext return ErrInvalidContext
} }
c.cancel()
c.wg.Wait()
return c.cancelErr return c.cancelErr
} }
@ -206,7 +214,7 @@ func (c *Context) newSession(ctx context.Context) error {
for _, enable := range []Action{ for _, enable := range []Action{
log.Enable(), log.Enable(),
runtime.Enable(), runtime.Enable(),
//network.Enable(), // network.Enable(),
inspector.Enable(), inspector.Enable(),
page.Enable(), page.Enable(),
dom.Enable(), dom.Enable(),

View File

@ -28,7 +28,7 @@ var (
func testAllocate(t *testing.T, path string) (_ context.Context, cancel func()) { func testAllocate(t *testing.T, path string) (_ context.Context, cancel func()) {
// Same browser, new tab; not needing to start new chrome browsers for // Same browser, new tab; not needing to start new chrome browsers for
// each test gives a huge speed-up. // each test gives a huge speed-up.
ctx, cancel_ := NewContext(browserCtx) ctx, _ := NewContext(browserCtx)
// Only navigate if we want a path, otherwise leave the blank page. // Only navigate if we want a path, otherwise leave the blank page.
if path != "" { if path != "" {
@ -38,8 +38,7 @@ func testAllocate(t *testing.T, path string) (_ context.Context, cancel func())
} }
cancelErr := func() { cancelErr := func() {
cancel_() // not cancel(); that's the returned one if err := Cancel(ctx); err != nil {
if err := CancelError(ctx); err != nil {
t.Error(err) t.Error(err)
} }
} }
@ -180,8 +179,7 @@ func TestCancelError(t *testing.T) {
if err := Run(ctx2); err != nil { if err := Run(ctx2); err != nil {
t.Fatal(err) t.Fatal(err)
} }
cancel2() if err := Cancel(ctx2); err != nil {
if err := CancelError(ctx2); err != nil {
t.Fatalf("expected a nil error, got %v", err) t.Fatalf("expected a nil error, got %v", err)
} }
@ -192,9 +190,7 @@ func TestCancelError(t *testing.T) {
t.Fatal(err) t.Fatal(err)
} }
FromContext(ctx3).Target.TargetID = "wrong" FromContext(ctx3).Target.TargetID = "wrong"
cancel3() if err := Cancel(ctx3); err == nil {
time.Sleep(time.Second)
if err := CancelError(ctx3); err == nil {
t.Fatalf("expected a non-nil error, got %v", err) t.Fatalf("expected a non-nil error, got %v", err)
} }
} }

View File

@ -347,7 +347,7 @@ func TestLoadIframe(t *testing.T) {
// TODO: remove the sleep once we have better support for // TODO: remove the sleep once we have better support for
// iframes. // iframes.
Sleep(10 * time.Millisecond), Sleep(10 * time.Millisecond),
//WaitVisible(`#form`, ByID), // for the nested form.html // WaitVisible(`#form`, ByID), // for the nested form.html
}); err != nil { }); err != nil {
t.Fatal(err) t.Fatal(err)
} }