Refactor force browser close on panic #755

ankur22 · 2023-02-06T18:30:54Z

We're currently storing the browser pid in the context. This is an issue because:

When working on Return an error from BrowserContext.AddCookies and handle panic in mapping layer #744 it wasn't cleanly possible to panic at the mapping layer and kill all the browser processes since the pid was in a child context which we didn't easily have access to.
If a user launches more than one browser, and a panic occurs, only the last launched browser will be closed since that's the only pid in the context.

This change adds a register which is internally updated by BrowserProcess. When there is a need to panic (with k6ext.Panic) we can use the register's ForceProcessShutdown, which will loop over all the registered pids and force kill the processes.

ankur22 · 2023-02-07T14:50:38Z

browserprocess/register_test.go

@@ -1,57 +0,0 @@
-package browserprocess


This became a bit meaningless after removing unregister. Testing this is a little tricky 🤔 Does need tests though.

inancgumus

Nice idea 👍 I'll take a look at this in detail later. Here's one suggestion after skimming the PR.

browserprocess/browser_process.go

ka3de

Thanks for working on this @ankur22 ! Added a couple of comments.

browserprocess/register.go

ka3de

I am providing a suggestion with which I was experimenting in order to avoid the usage of string concatenation for iterationID-pid relationship. Feel free to discard it or discuss just some options of it. We take advantage of the fact that when not using test_browser, we'll have one "default iteration ID" which is a void string. Or we could even fix that in the GetIterationID method.
Let me know if I misunderstood something.

browserprocess/register.go

This rename better defines what the package does. Resolves: #755 (comment)

tests/test_browser.go

osext/browser_process.go

Helps distinguish what is being created now that the package is called osext instead of browserprocess. Resolves: #755 (comment)

This will avoid any confusion when debugging the register -- an empty RunID in the register might confuse devs, whereas default shouldn't and can be tracked to where it was set. Resolves: #755 (comment)

This rename better defines what the package does. Resolves: #755 (comment)

This renaming better reflects what this type is and how we can use it. It should avoid any confusion as to it being part of the external JS API. Resolves: #755 (comment)

Helps distinguish what is being created now that the package is called osext instead of browserprocess. Resolves: #755 (comment)

This will avoid any confusion when debugging the register -- an empty RunID in the register might confuse devs, whereas default shouldn't and can be tracked to where it was set. Resolves: #755 (comment)

Before we can refactor to work with a registering system for the pid that can help us to shutdown all the browser processes. We need to abstract away the BrowserProcess into its own package to avoid cyclic dependencies.

Since BrowserProcessor is in a package called browserprocess we can rename the new method to just New.

This process register will store all the currently running browser pids that xk6-browser has started in the current test iteration. It's main use is to be able to quickly and easily shutdown these processes if a panic occurs. This negates the need to store the pid in the context. This also solves an issue where if more than one browser was started, only the last one could be forced to shutdown.

The BrowserProcess will register the pid in the register as it seems most capable of doing so and it is the closest to the os process.

Remove the need to write and read to the context. Now we can use the method ForceProcessShutdown to force shutdown the browser instances.

Although the register works for a normal test run with multiple browser where a panic will warrant the closure of all the browser instance, this is not the same for unit tests. Unit tests share the same register, which is a problem since if one test run panics, then all currently open browser instances will close. To fix this, register is now aware of interval ids which is a way for unit tests to isolate panics within their own test. Normal running of xk6-browser doesn't require an interval id.

Co-authored-by: ka3de <daniel.jimenez@grafana.com>

This rename better defines what the package does. Resolves: #755 (comment)

This renaming better reflects what this type is and how we can use it. It should avoid any confusion as to it being part of the external JS API. Resolves: #755 (comment)

Helps distinguish what is being created now that the package is called osext instead of browserprocess. Resolves: #755 (comment)

We already have an iterationID, so to avoid confusion, it's being renamed to runID.

This will avoid any confusion when debugging the register -- an empty RunID in the register might confuse devs, whereas default shouldn't and can be tracked to where it was set. Resolves: #755 (comment)

This will prevent integration tests from running the os.Kill command on the browser instances that are open. Instead they will be gracefully shutdown, as was the assumption when these tests were written.

This was only needed to prevent one integration tests from affecting other integration tests if it panicked. Now that we're not allowing os.Kill in integration tests this runID isn't needed.

inancgumus

Yes, i've been thinking about this too. I'm not a fan of doing it this way, but as far as I can tell there's no nice way to inject a register instance to both the osext.Process (which is registering the browser pids) and k6ext.Panic without affecting the current API and a lot of code.

Please see #779.

ankur22 requested review from inancgumus and ka3de February 6, 2023 18:30

ankur22 mentioned this pull request Feb 6, 2023

Return an error from BrowserContext.AddCookies and handle panic in mapping layer #744

Closed

ankur22 force-pushed the refactor/use-bpr branch 2 times, most recently from 8a11c31 to 6407ceb Compare February 6, 2023 23:25

ankur22 marked this pull request as draft February 6, 2023 23:31

ankur22 removed request for inancgumus and ka3de February 6, 2023 23:31

ankur22 force-pushed the refactor/use-bpr branch 3 times, most recently from 1e3c2c0 to 3afac7d Compare February 7, 2023 10:14

ankur22 added the refactor label Feb 7, 2023

ankur22 self-assigned this Feb 7, 2023

ankur22 added this to the v0.9.0 milestone Feb 7, 2023

ankur22 force-pushed the refactor/use-bpr branch 3 times, most recently from c688fd2 to d8f1ed9 Compare February 7, 2023 14:49

ankur22 commented Feb 7, 2023

View reviewed changes

ankur22 force-pushed the refactor/use-bpr branch from d8f1ed9 to efc5107 Compare February 7, 2023 15:42

ankur22 requested review from inancgumus and ka3de February 7, 2023 15:46

ankur22 marked this pull request as ready for review February 7, 2023 15:46

inancgumus reviewed Feb 7, 2023

View reviewed changes

browserprocess/browser_process.go Outdated Show resolved Hide resolved

ka3de reviewed Feb 8, 2023

View reviewed changes

browserprocess/register.go Outdated Show resolved Hide resolved

browserprocess/register.go Outdated Show resolved Hide resolved

ka3de reviewed Feb 8, 2023

View reviewed changes

browserprocess/register.go Outdated Show resolved Hide resolved

browserprocess/register.go Outdated Show resolved Hide resolved

browserprocess/register.go Outdated Show resolved Hide resolved

browserprocess/register.go Outdated Show resolved Hide resolved

ankur22 force-pushed the refactor/use-bpr branch from 30ce3df to ad33a2d Compare February 8, 2023 09:26

ankur22 added a commit that referenced this pull request Feb 8, 2023

Rename browserprocess to osext

edb07ac

This rename better defines what the package does. Resolves: #755 (comment)

ankur22 force-pushed the refactor/use-bpr branch from a1f72e4 to edb07ac Compare February 8, 2023 09:38

ankur22 commented Feb 8, 2023

View reviewed changes

tests/test_browser.go Outdated Show resolved Hide resolved

ka3de reviewed Feb 8, 2023

View reviewed changes

osext/browser_process.go Outdated Show resolved Hide resolved

ankur22 added a commit that referenced this pull request Feb 10, 2023

Rename from New to NewProcess

115dbcb

Helps distinguish what is being created now that the package is called osext instead of browserprocess. Resolves: #755 (comment)

ankur22 force-pushed the refactor/use-bpr branch from f5a50ea to f86b6eb Compare February 10, 2023 16:18

ankur22 added a commit that referenced this pull request Feb 10, 2023

Rename browserprocess to osext

2f232c5

This rename better defines what the package does. Resolves: #755 (comment)

ankur22 added a commit that referenced this pull request Feb 10, 2023

Rename from BrowserProcess to Process

fc9a6e4

This renaming better reflects what this type is and how we can use it. It should avoid any confusion as to it being part of the external JS API. Resolves: #755 (comment)

ankur22 added a commit that referenced this pull request Feb 10, 2023

Rename from New to NewProcess

060f771

Helps distinguish what is being created now that the package is called osext instead of browserprocess. Resolves: #755 (comment)

ankur22 force-pushed the refactor/use-bpr branch from f86b6eb to f309af9 Compare February 10, 2023 16:25

ankur22 and others added 14 commits February 10, 2023 16:25

Refactor BrowserProcess into its own package

227d21f

Before we can refactor to work with a registering system for the pid that can help us to shutdown all the browser processes. We need to abstract away the BrowserProcess into its own package to avoid cyclic dependencies.

Refactor NewBrowserProcess to New

3f7d2c5

Since BrowserProcessor is in a package called browserprocess we can rename the new method to just New.

Add calls register

0c3b911

The BrowserProcess will register the pid in the register as it seems most capable of doing so and it is the closest to the os process.

Remove & use browserprocess.ForceProcessShutdown

51af7ed

Remove the need to write and read to the context. Now we can use the method ForceProcessShutdown to force shutdown the browser instances.

Apply suggestions from code review

8bbc3ab

Co-authored-by: ka3de <daniel.jimenez@grafana.com>

Rename browserprocess to osext

80aa482

This rename better defines what the package does. Resolves: #755 (comment)

Rename from BrowserProcess to Process

ee6e1a3

This renaming better reflects what this type is and how we can use it. It should avoid any confusion as to it being part of the external JS API. Resolves: #755 (comment)

Rename from New to NewProcess

96fb1f2

Helps distinguish what is being created now that the package is called osext instead of browserprocess. Resolves: #755 (comment)

Rename from iterationID to runID

bcbce68

We already have an iterationID, so to avoid confusion, it's being renamed to runID.

Add a defaultRunID

043a5d2

This will avoid any confusion when debugging the register -- an empty RunID in the register might confuse devs, whereas default shouldn't and can be tracked to where it was set. Resolves: #755 (comment)

Add exported method to not kill in int. tests

7cc74c7

This will prevent integration tests from running the os.Kill command on the browser instances that are open. Instead they will be gracefully shutdown, as was the assumption when these tests were written.

Remove the runID code

b901685

This was only needed to prevent one integration tests from affecting other integration tests if it panicked. Now that we're not allowing os.Kill in integration tests this runID isn't needed.

ankur22 force-pushed the refactor/use-bpr branch from f309af9 to b901685 Compare February 10, 2023 16:25

inancgumus mentioned this pull request Feb 15, 2023

Clear zombie browser processes on panic #779

Merged

inancgumus self-requested a review February 15, 2023 23:54

inancgumus requested changes Feb 15, 2023

View reviewed changes

ankur22 closed this Feb 21, 2023

ankur22 deleted the refactor/use-bpr branch February 21, 2023 14:56

inancgumus removed this from the v0.9.0 milestone Feb 24, 2023

ka3de mentioned this pull request Mar 1, 2023

Implement BrowserType.connect #800

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor force browser close on panic #755

Refactor force browser close on panic #755

ankur22 commented Feb 6, 2023

ankur22 Feb 7, 2023 •

edited

inancgumus left a comment

ka3de left a comment

ka3de left a comment

inancgumus left a comment •

edited

Refactor force browser close on panic #755

Refactor force browser close on panic #755

Conversation

ankur22 commented Feb 6, 2023

ankur22 Feb 7, 2023 • edited

Choose a reason for hiding this comment

inancgumus left a comment

Choose a reason for hiding this comment

ka3de left a comment

Choose a reason for hiding this comment

ka3de left a comment

Choose a reason for hiding this comment

inancgumus left a comment • edited

Choose a reason for hiding this comment

ankur22 Feb 7, 2023 •

edited

inancgumus left a comment •

edited