Repository navigation
Fix dispose and getContext data race - #6327
Conversation
Signed commits reportAll 4 commits between |
|
|
JohnPei1
changed the title
44a8ae0 to
eaf0acb
Compare
ankur22
left a comment
ankur22
left a comment
There was a problem hiding this comment.
Could we update the contextMu comment as part of this? It currently says the mutex protects only the NewContext/getDefaultBrowserContextOrMatchedID edge case and “this one case,” but disposeContext now relies on it too. That no longer describes the locking invariant.
| if err := action.Do(cdp.WithExecutor(b.vuCtx, b.conn)); err != nil { | ||
| return fmt.Errorf("disposing browser context ID %s: %w", id, err) | ||
| } | ||
| b.contextMu.Lock() |
There was a problem hiding this comment.
This synchronizes the write with getDefaultBrowserContextOrMatchedID, but Browser.Context() still returns b.context without the mutex; CloseContext() and NewContext() also read it directly. Since browser.context() is synchronous while closeContext() runs in a promise goroutine, the disposal write can still race with those reads. Could we route every access to b.context through contextMu and cover the public getter in the regression test?
There was a problem hiding this comment.
Updated all context read accesses to route through the Context() getter and added read lock to the getter. This should fix the race between the disposal write and context reads.
However, one concern is that methods like CloseContext() and NewContext() still operate on a snapshot of the current context after releasing contextMu. And it could lead to undesirable results like Close() being called on a disposed context. I wonder if we should add a new mutex to serialize all close/create operations, and whether we should open a separate ticket for that purpose.
| go func() { | ||
| defer wg.Done() | ||
| for range iterations { | ||
| _ = b.disposeContext(browserContextID) |
There was a problem hiding this comment.
Could we add assertions for the operation results here? At the moment the dispose error and lookup result are both discarded, so without -race this only proves that the goroutines finished. I’d collect the results and, after wg.Wait(), verify that disposal succeeded, every lookup returned a non-nil context, and the final context is nil/default. A NotPanics wrapper wouldn’t catch panics from these worker goroutines; an unrecovered panic already fails the test anyway.
There was a problem hiding this comment.
Good call. Expanded the regression test to assert return values and states, also included the public getter in the test and updated the stale comment on contextMu.
GitHub Actions
Inactive
GitHub Actions
Inactive
953206e to
3d2324f
Compare
GitHub Actions
Inactive
GitHub Actions
Inactive
|
@JohnPei1 Can you sign all your commits so we can get this PR merged please? Sorry if it was the case before, I may have lost the signature when I rebased your PR to get the CI to pass. |
3d2324f to
6749c62
Compare
No worries, should (hopefully) be good now! |
GitHub Actions
Inactive
GitHub Actions
Inactive
What?
Fixes a data race between
BrowserContext.Close()andBrowserContext.NewPage().Added read lock to the context getter
Context(), and updated read accesses to the browser context to use the getter .Adds a regression test that runs context disposal and context lookup concurrently
Why?
BrowserContext.Close() sets b.context = nil without acquiring any lock, which causes getDefaultBrowserContextOrMatchedID() to return a nil b.context when interleaved in a certain way.
The new test, when ran with
go test -race ./internal/js/modules/k6/browser/commonbefore the fix, shows the data raceChecklist
make check) and all pass.Checklist: Documentation (only for k6 maintainers and if relevant)
Please do not merge this PR until the following items are filled out.
Related PR(s)/Issue(s)
Closes #5328