Skip to content

Commit

Permalink
fix a data race when Cancel results in an os.RemoveAll error
Browse files Browse the repository at this point in the history
The data race below was caught by a test in a third-party project:

	WARNING: DATA RACE
	Write at 0x00c0001564b8 by goroutine 27:
	  github.com/chromedp/chromedp.(*ExecAllocator).Allocate.func2()
	      github.com/chromedp/chromedp@v0.5.4-0.20200615134453-af6afb6eba43/allocate.go:209 +0x109

	Previous read at 0x00c0001564b8 by goroutine 40:
	  github.com/chromedp/chromedp.Cancel()
	      github.com/chromedp/chromedp@v0.5.4-0.20200615134453-af6afb6eba43/chromedp.go:227 +0x1e8

The cause was that c.cancelErr can be modified while c.allocated hasn't
been closed, and Cancel can end up reading c.cancelErr before the
channel is closed if the context is canceled.

Fix that mistake. Before, the third-party test failed 4 times out of 600
with the stress tool and -race. After the fix, we got to 1000 runs with
no failures.
  • Loading branch information
mvdan committed Jul 29, 2020
1 parent c101504 commit ccb1bb0
Showing 1 changed file with 8 additions and 0 deletions.
8 changes: 8 additions & 0 deletions chromedp.go
Expand Up @@ -225,6 +225,14 @@ func Cancel(ctx context.Context) error {
if graceful {
c.cancel()
}

// If we allocated and we hit ctx.Done earlier, we can't rely on
// cancelErr being ready until the allocated channel is closed, as that
// is racy. If we didn't hit ctx.Done earlier, then c.allocated was
// already cancelled then, so this will be a no-op.
if c.allocated != nil {
<-c.allocated
}
return c.cancelErr
}

Expand Down

0 comments on commit ccb1bb0

Please sign in to comment.