-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix browser: ignore context cancellation during target attachment - Close #5504 #5526
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
fix browser: ignore context cancellation during target attachment - Close #5504 #5526
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LBaronceli we've had some discussions and I've played around with your change, and indeed it helps us avoid a panic which can cause various other issues in the integration tests (data races) as well as when running k6 normally (panic causing the test to fail unnecessarily).
Could you take a look at the error that the linter is pointing to and resolve it? I believe we should be able to work with something like:
err := ...
if err is context canceled {
log
continue
}
if err not nil {
panic
}WDYT?
…ted if block lint issues
|
Hey Ankur, Sure! made sure the if statements aren't nested, and added a debug log in case the continue is triggered by the Canceled or DealineExceeded errors. Thank you for you patience and help with this one! |
ankur22
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brilliant! Thanks for digging into this @LBaronceli and helping us with the fix 🙇 🚀
What?
Fixes a race during browser shutdown where Browser.onAttachedToTarget could attempt to create new pages or sessions after the browser context had already been canceled.
The change ensures that context cancellation during teardown is handled gracefully and does not surface as a fatal error or panic.
Why?
Under high concurrency (e.g. repeated test runs or low GOMAXPROCS), Target.attachedToTarget events can still be delivered while the browser is shutting down.
In this situation, attempting to create new pages leads to panics or test failures, even though shutdown is expected behavior.
This change aligns Browser.onAttachedToTarget with existing defensive handling elsewhere in the browser codebase by treating context cancellation as a non-fatal condition.
Checklist
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 #5504