Pass and respect a `Context` when initializing VUs #2800

na-- · 2022-12-05T12:33:46Z

This closes #2790. It is not very urgent, it doesn't need to be released in v0.42.0, instead it is meant to be merged some time early in the v0.43.0 cycle.

Most of the LoC changes are just boilerplate churn, mostly test and API signature adjustments. They have been grouped together into the first 2 commits of the PR and should contain no functional or test changes.

The important changes are focused in the next 2 commits, which should be the reviewed carefully. I may add some more tests there, though I am not sure what they should be 🤔 If anyone has any ideas, please share 🙏

codecov-commenter · 2022-12-05T12:44:29Z

Codecov Report

Merging #2800 (b641de7) into master (8418147) will decrease coverage by 0.18%.
The diff coverage is 100.00%.

❗ Current head b641de7 differs from pull request most recent head 9c9e35f. Consider uploading reports for the commit 9c9e35f to get more accurate results

@@            Coverage Diff             @@
##           master    #2800      +/-   ##
==========================================
- Coverage   76.04%   75.85%   -0.19%     
==========================================
  Files         213      211       -2     
  Lines       16554    16577      +23     
==========================================
- Hits        12588    12575      -13     
- Misses       3195     3223      +28     
- Partials      771      779       +8

Flag	Coverage Δ
ubuntu	`?`
windows	`75.85% <100.00%> (+0.09%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
js/initcontext.go	`88.18% <ø> (ø)`
core/local/local.go	`89.70% <100.00%> (-0.45%)`	⬇️
js/bundle.go	`90.62% <100.00%> (+0.62%)`	⬆️
js/runner.go	`85.26% <100.00%> (+0.07%)`	⬆️
lib/testutils/minirunner/minirunner.go	`82.43% <100.00%> (+0.24%)`	⬆️
lib/netext/tls.go	`27.08% <0.00%> (-22.92%)`	⬇️
api/v1/metric.go	`63.63% <0.00%> (-22.73%)`	⬇️
loader/readsource.go	`75.86% <0.00%> (-10.35%)`	⬇️
metrics/value_type.go	`62.50% <0.00%> (-6.25%)`	⬇️
lib/executor/vu_handle.go	`92.52% <0.00%> (-5.61%)`	⬇️
... and 13 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

imiric

LGTM from a first look, will make another pass later.

Nice job splitting it into atomic commits, makes reviewing much easier 👍

As for additional tests, and maybe I'm missing it, but I think there should be a test replicating the user pressing ^C during VU initialization. I see a couple of tests sending os.Interrupt, but it happens while the test is running. The tests aborted in init should cover the same code paths, but receiving SIGINT should trigger it as well.

js/bundle.go

na-- · 2022-12-14T11:43:57Z

@imiric, I was super surprised that a test didn't exist, and when I started to add it, I realized why I hadn't done it originally 😅

This PR makes it so VU initializations are going to be gracefully aborted with Ctrl+C, true. However, k6 won't return the right exit code or send the run_status value yet... 😞 IIRC, this is when I started writing #2804 😅

Still, I decided to go ahead and add a test anyway, even if it needs to check the current values of these... I just added a couple of TODOs, so we know the test doesn't verify the desired behavior, but the current one.

So, I cherry-picked one commit (d5124d1) from #2807, to simplify the integration tests that use Ctrl+C or REST API to stop k6, and then added ed39495 with the actual test to check what happens when an interrupt signal is given while VUs are initializing.

codebien

Thanks for doing it, I saw very nice improvements. 🙇 I was reviewing when you force-pushed so something could be lost on my side.

js/initcontext.go

js/runner.go

codebien · 2022-12-14T11:49:12Z

js/bundle.go

+	// TODO: make something cleaner for interrupting scripts, and more unified
+	// (e.g. as a part of the event loop or RunWithPanicCatching()?
+	initCtxDone := init.moduleVUImpl.ctx.Done()
+	initDone := make(chan struct{})
+	watchDone := make(chan struct{})
+	go func() {
+		select {
+		case <-initCtxDone:
+			rt.Interrupt(init.moduleVUImpl.ctx.Err())


I need to check what the logic is in the other cases (not init) but if it is the same maybe could we delegate this listener directly in moduleVUImpl? 🤔

If the logic is different, we should use this opportunity to wrap the error so we know it is generated by the init process.

Unfortunately it wasn't straightforward to unify this init interrupt logic with the rest of the test abort logic during test execution 😞 At least I didn't see how to do that without a much, much bigger refactoring of the whole js/ package... 😞

So I chose to make only minor improvements in this PR (see the Simplify the js/Bundle.instantiate() API commit) and leave the rest for a future PR (see the TODO in the code snippet you've selected).

If the logic is different, we should use this opportunity to wrap the error so we know it is generated by the init process.

What about this, is it doable?

Hmm not sure what additional information we can add here, as the rest of the code is now 🤔 I think it will be much easier to fix this after we have the NewTestRunContext() in a "higher" place, i.e. after #2813 🤔

js/initcontext.go

js/runner.go

codebien · 2022-12-14T13:06:34Z

js/bundle.go

+	// TODO: make something cleaner for interrupting scripts, and more unified
+	// (e.g. as a part of the event loop or RunWithPanicCatching()?
+	initCtxDone := init.moduleVUImpl.ctx.Done()
+	initDone := make(chan struct{})
+	watchDone := make(chan struct{})
+	go func() {
+		select {
+		case <-initCtxDone:
+			rt.Interrupt(init.moduleVUImpl.ctx.Err())


If the logic is different, we should use this opportunity to wrap the error so we know it is generated by the init process.

What about this, is it doable?

imiric

LGTM 👍

Just left a minor comment about test helpers.

imiric · 2022-12-14T15:47:58Z

cmd/integration_test.go

+	})
+}
+
+func asyncWaitForStdoutAndStopTestFromRESTAPI(


This function is only called from TestAbortedByUserWithRestAPI, and just wraps another helper function. So I would inline the asyncWaitForStdoutAndRun() call, and remove this.

asyncWaitForStdoutAndRun() is also called with the same arguments, except for the callback func. So I would drop the unneded arguments, and add them once they're needed.

In general, abstractions add a layer of indirection that complicates code comprehension, so I wouldn't use them so liberally, and would prefer even some duplication between tests. Or, if tests really are similar, turn them into table tests and avoid the helper functions.

I agree and I was about to fix this, but I think the "fix" here is to just add more tests that abort the test run via the k6 REST API 😅 See #2804, we need more tests about this behavior... 😞

In #2815 or soon after it, we should be able to nicely abort the test with the REST even during VU init or setup(), so I'd prefer to keep this helper method here and add more tests that use it in that PR.

With some extra work, this will allow us to gracefully interrupt VUs in the middle of their initialization.

As I've mentioned in the code comments, this test doesn't test the eventually desired behavior, it tests the currently expected behavior as it is. The current behavior is still much better than the behavior before this PR, where the VU init was interrupted without any handling and graceful stops. However, the fix to actually abort k6 with the correct exit code and run_status will be in another PR.

codebien

LGTM

na-- added this to the v0.43.0 milestone Dec 5, 2022

na-- requested review from mstoykov and imiric December 5, 2022 12:33

na-- force-pushed the fix-run-error-wait branch 2 times, most recently from cea7276 to a8ff4a1 Compare December 5, 2022 15:41

Base automatically changed from fix-run-error-wait to master December 5, 2022 16:26

na-- force-pushed the init-vus-with-context branch 2 times, most recently from 8398f3f to 11564b3 Compare December 5, 2022 21:45

na-- mentioned this pull request Dec 5, 2022

Prevent real requests to the cloud during integration tests #2802

Merged

na-- force-pushed the init-vus-with-context branch from 11564b3 to 3c041d2 Compare December 5, 2022 22:02

na-- mentioned this pull request Dec 5, 2022

Ensure REST API server is shut down after test ends and check for goroutine leaks #2803

Merged

na-- changed the base branch from master to prevent-real-network-requests-in-tests December 5, 2022 23:09

Base automatically changed from prevent-real-network-requests-in-tests to master December 6, 2022 08:29

na-- force-pushed the init-vus-with-context branch from 3c041d2 to ff2a8e4 Compare December 6, 2022 08:31

na-- mentioned this pull request Dec 6, 2022

Unify and standardize behavior and exit codes when k6 is stopped #2804

Open

na-- force-pushed the init-vus-with-context branch from ff2a8e4 to dbde3c5 Compare December 7, 2022 09:21

na-- changed the base branch from master to master-next December 7, 2022 09:21

na-- added bug tests labels Dec 8, 2022

na-- mentioned this pull request Dec 9, 2022

[WIP] PoC for native distributed execution #2438

Closed

na-- requested a review from codebien December 9, 2022 11:19

imiric reviewed Dec 13, 2022

View reviewed changes

js/bundle.go Outdated Show resolved Hide resolved

mstoykov previously approved these changes Dec 13, 2022

View reviewed changes

na-- force-pushed the init-vus-with-context branch from dbde3c5 to ed39495 Compare December 14, 2022 11:39

na-- requested a review from imiric December 14, 2022 11:45

na-- mentioned this pull request Dec 14, 2022

Use non-zero exit codes for some aborted by Ctrl+C or REST API tests #2807

Merged

codebien reviewed Dec 14, 2022

View reviewed changes

na-- requested a review from codebien December 14, 2022 12:16

na-- force-pushed the init-vus-with-context branch from 47bd057 to f0962a9 Compare December 14, 2022 12:20

codebien requested changes Dec 14, 2022

View reviewed changes

imiric previously approved these changes Dec 14, 2022

View reviewed changes

imiric mentioned this pull request Dec 16, 2022

Refactor integration tests #2821

Merged

Base automatically changed from master-next to master December 20, 2022 16:09

imiric dismissed mstoykov’s stale review via f0962a9 December 20, 2022 16:09

na-- mentioned this pull request Dec 23, 2022

Refactor console UI handling outside of the cmd package #2787

Merged

na-- mentioned this pull request Jan 3, 2023

CSV output: vu and iter system tags not output after v0.40.0 #2827

Closed

na-- added 6 commits January 10, 2023 16:43

Thread a real context when initializing new VUs

24cbf79

With some extra work, this will allow us to gracefully interrupt VUs in the middle of their initialization.

Simplify the js/Bundle.instantiate() API

d459657

Interrupt the JS runtime during VU init if the context was cancelled

7b70cf1

Ensure all VUs are gracefully aborted when an init error occurs

e7a1cc3

Simplify and improve mid-test abort mechanisms for integration tests

384bf5c

na-- dismissed imiric’s stale review via 9c9e35f January 10, 2023 14:44

na-- force-pushed the init-vus-with-context branch from f0962a9 to 9c9e35f Compare January 10, 2023 14:44

Improve code and comments from PR suggestions

a7ad53f

na-- force-pushed the init-vus-with-context branch from 9c9e35f to a7ad53f Compare January 10, 2023 15:08

na-- requested review from codebien, imiric and mstoykov January 10, 2023 15:08

codebien approved these changes Jan 10, 2023

View reviewed changes

mstoykov approved these changes Jan 11, 2023

View reviewed changes

na-- merged commit e9d390d into master Jan 11, 2023

na-- deleted the init-vus-with-context branch January 11, 2023 13:29

mstoykov mentioned this pull request Feb 17, 2023

rare Panic during initialization of VUs #2917

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass and respect a `Context` when initializing VUs #2800

Pass and respect a `Context` when initializing VUs #2800

na-- commented Dec 5, 2022

codecov-commenter commented Dec 5, 2022 •

edited

Loading

imiric left a comment

na-- commented Dec 14, 2022 •

edited

Loading

codebien left a comment •

edited

Loading

codebien Dec 14, 2022

na-- Dec 14, 2022

codebien Dec 14, 2022

na-- Jan 10, 2023

codebien Dec 14, 2022

imiric left a comment

imiric Dec 14, 2022

na-- Jan 10, 2023 •

edited

Loading

codebien left a comment

Pass and respect a Context when initializing VUs #2800

Pass and respect a Context when initializing VUs #2800

Conversation

na-- commented Dec 5, 2022

codecov-commenter commented Dec 5, 2022 • edited Loading

Codecov Report

imiric left a comment

Choose a reason for hiding this comment

na-- commented Dec 14, 2022 • edited Loading

codebien left a comment • edited Loading

Choose a reason for hiding this comment

codebien Dec 14, 2022

Choose a reason for hiding this comment

na-- Dec 14, 2022

Choose a reason for hiding this comment

codebien Dec 14, 2022

Choose a reason for hiding this comment

na-- Jan 10, 2023

Choose a reason for hiding this comment

codebien Dec 14, 2022

Choose a reason for hiding this comment

imiric left a comment

Choose a reason for hiding this comment

imiric Dec 14, 2022

Choose a reason for hiding this comment

na-- Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

codebien left a comment

Choose a reason for hiding this comment

Pass and respect a `Context` when initializing VUs #2800

Pass and respect a `Context` when initializing VUs #2800

codecov-commenter commented Dec 5, 2022 •

edited

Loading

na-- commented Dec 14, 2022 •

edited

Loading

codebien left a comment •

edited

Loading

na-- Jan 10, 2023 •

edited

Loading