chromedp.Cancel and cancel from chromedp.NewContext do a different thing

[karelbilek]

chromedp.Cancel and chromedp.NewContext cancel function behave very differently, which is not at first obvious from the documentation.

The documentation mentions that chromedp.Cancel is "checking for errors", but that is not all the difference.

• chromedp.NewContext cancel can be called multiple times; chromedp.Cancel panics if called for the second time, as it tries to close a closed channel twice
• chromedp.Cancel closes the browser gracefully and then waits for the process to end; chromedp.NewContext cancel sends sigkill to the browser (through exec.CommandContext). That matters if chrome is run through a wrapper script, as the sigkill is sent to the script, which does not kill the sub-processes; and if the script has some clean-up, it cannot run that either.
• chromedp.Cancel can hang, if the browser is not responsive; chromedp.NewContext cancel never hangs, as it just kills the process

This all applies only when the context is the first tab. I am not sure what are the differences if the context is for a second tab.

I am not sure if this is desired behavior or not.

It is not documented, but I am not sure if it's better to change the documentation or change the behavior so the two functions are consistent and only differ in error checking (as that can introduce some subtle issues in existing code).

[karelbilek]

I wrote this function that fixes the following:

• it never hangs longer than few seconds
• it always closes even when browser is unresponsive
• it is safe to call from more goroutines
• it is safe to call multiple times
• it lets the script to finish cleanup (if it is under 5 seconds)
• still does the error checking (so I can use it for both simple defer or where I need to check errors)

ctx, cancel := chromedp.NewContext(allocCtx)
var mutex sync.Mutex
newCancel := func() error {
	mutex.Lock()
	defer mutex.Unlock()
	if ctx.Err() == nil {
		deadlineCtx, deadlineCancel := context.WithTimeout(ctx, 5*time.Second)
		defer deadlineCancel()
		err := chromedp.Cancel(deadlineCtx)
		cancel()
		return err
	}
	return ctx.Err()
}

I am not sure if it is usable by default as cancelation func from chromedp.NewContext though, not sure what depends on it etc

[ZekeLu]

Have you read the docs below?

chromedp/chromedp.go

Lines 84 to 98 in 4b483eb

	// NewContext creates a chromedp context from the parent context. The parent
	// context's Allocator is inherited, defaulting to an ExecAllocator with
	// DefaultExecAllocatorOptions.
	//
	// If the parent context contains an allocated Browser, the child context
	// inherits it, and its first Run creates a new tab on that browser. Otherwise,
	// its first Run will allocate a new browser.
	//
	// Cancelling the returned context will close a tab or an entire browser,
	// depending on the logic described above. To cancel a context while checking
	// for errors, see Cancel.
	//
	// Note that NewContext doesn't allocate nor start a browser; that happens the
	// first time Run is used on the context.
	func NewContext(parent context.Context, opts ...ContextOption) (context.Context, context.CancelFunc) {

chromedp/chromedp.go

Lines 181 to 195 in 4b483eb

	// Cancel cancels a chromedp context, waits for its resources to be cleaned up,
	// and returns any error encountered during that process.
	//
	// If the context allocated a browser, the browser will be closed gracefully by
	// Cancel. A timeout can be attached to this context to determine how long to
	// wait for the browser to close itself:
	//
	// tctx, tcancel := context.WithTimeout(ctx, 10 * time.Second)
	// defer tcancel()
	// chromedp.Cancel(tctx)
	//
	// Usually a "defer cancel()" will be enough for most use cases. However, Cancel
	// is the better option if one wants to gracefully close a browser, or catch
	// underlying errors happening during cancellation.
	func Cancel(ctx context.Context) error {

Basically, I will just call the cancel func to cancel the context. And if I need the browser to be closed gracefully, I will call chromedp.Cancel explicitly (that's very rare though. See one use case here #818).

That matters if chrome is run through a wrapper script, as the sigkill is sent to the script, which does not kill the sub-processes; and if the script has some clean-up, it cannot run that either.

Do you have a concrete use case? Does #674 help in this case?

[karelbilek]

 // Cancelling the returned context will close a tab or an entire browser, 
 // depending on the logic described above. To cancel a context while checking 
 // for errors, see Cancel. 

This does not tell anything about the subtle differences of cancel vs. Cancel that I listed above

Do you have a concrete use

Yep I use xvfb-chromium, adapted from https://github.com/atlassian/docker-chromium-xvfb/blob/master/images/base/xvfb-chromium (changed somewhat), that kills the xvfb afterwards.

[ZekeLu]

 // Usually a "defer cancel()" will be enough for most use cases. However, Cancel 
 // is the better option if one wants to gracefully close a browser, or catch 
 // underlying errors happening during cancellation. 

It has been mentioned here. I don't know how to improve the doc. Do you want to send a PR to improve it?

WRT xvfb, can you try whether chromedp.ModifyCmdFunc introduced by #674 help in this case?

[karelbilek]

OK, so, this behavior is expected. So I will send PRs to improve the docs.

Should calling Cancel twice panic though? 🤔

[ZekeLu]

I think calling Cancel twice on the same context is a wrong use, it's the user's responsibility to make sure it's not called twice.

[karelbilek]

https://golang.org/pkg/context/#CancelFunc

In normal context, it works.

A CancelFunc tells an operation to abandon its work. A CancelFunc does not wait for the work to stop. A CancelFunc may be called by multiple goroutines simultaneously. After the first call, subsequent calls to a CancelFunc do nothing.

So it is a bit unexpected here. But OK, as you wish :)

[ZekeLu]

Please note that chromedp.Cancel is not a CancelFunc as defined by https://golang.org/pkg/context/#CancelFunc.

[ZekeLu]

I think the issue has been addressed. Closing.