Simple fix for 4240 (#4275)

grafana · Jan 28, 2025 · da7db7e · da7db7e
1 parent cfed5fc
commit da7db7e
Show file tree

Hide file tree

Showing 2 changed files with 10 additions and 16 deletions.
diff --git a/internal/js/modules/k6/browser/browser/registry.go b/internal/js/modules/k6/browser/browser/registry.go
@@ -10,7 +10,6 @@ import (
 	"strconv"
 	"strings"
 	"sync"
-	"sync/atomic"
 
 	"github.com/mstoykov/k6-taskqueue-lib/taskqueue"
 	"go.opentelemetry.io/otel/attribute"
@@ -181,8 +180,6 @@ type browserRegistry struct {
 	m  map[int64]*common.Browser
 
 	buildFn browserBuildFunc
-
-	stopped atomic.Bool // testing purposes
 }
 
 type browserBuildFunc func(ctx, vuCtx context.Context) (*common.Browser, error)
@@ -248,13 +245,13 @@ func newBrowserRegistry(
 	}
 
 	go r.handleExitEvent(exitCh, unsubscribe)
-	go r.handleIterEvents(ctx, eventsCh, unsubscribe)
+	go r.handleIterEvents(ctx, eventsCh)
 
 	return r
 }
 
 func (r *browserRegistry) handleIterEvents(
-	ctx context.Context, eventsCh <-chan *k6event.Event, unsubscribeFn func(),
+	ctx context.Context, eventsCh <-chan *k6event.Event,
 ) {
 	var (
 		ok   bool
@@ -268,12 +265,12 @@ func (r *browserRegistry) handleIterEvents(
 		// to each VU iter events, including VUs that do not make use of the browser in their
 		// iterations.
 		// Therefore, if we get an event that does not correspond to a browser iteration, then
-		// unsubscribe for the VU events and exit the loop in order to reduce unuseful overhead.
+		// skip this iteration. We can't just unsubscribe as the VU might be reused in a later
+		// scenario that does have browser setup.
+		// TODO try to maybe do this only once per scenario
 		if !isBrowserIter(r.vu) {
-			unsubscribeFn()
-			r.stop()
 			e.Done()
-			return
+			continue
 		}
 
 		// The context in the VU is not thread safe. It can
@@ -412,10 +409,6 @@ func (r *browserRegistry) stopTracesRegistry() {
 	}
 }
 
-func (r *browserRegistry) stop() {
-	r.stopped.Store(true)
-}
-
 func isBrowserIter(vu k6modules.VU) bool {
 	opts := k6ext.GetScenarioOpts(vu.Context(), vu)
 	_, ok := opts["type"] // Check if browser type option is set

diff --git a/internal/js/modules/k6/browser/browser/registry_test.go b/internal/js/modules/k6/browser/browser/registry_test.go
@@ -263,7 +263,7 @@ func TestBrowserRegistry(t *testing.T) {
 		assert.Equal(t, 0, browserRegistry.browserCount())
 	})
 
-	t.Run("unsubscribe_on_non_browser_vu", func(t *testing.T) {
+	t.Run("skip_on_non_browser_vu", func(t *testing.T) {
 		t.Parallel()
 
 		var (
@@ -277,9 +277,10 @@ func TestBrowserRegistry(t *testing.T) {
 		// a browser test VU
 		delete(vu.StateField.Options.Scenarios["default"].GetScenarioOptions().Browser, "type")
 
-		vu.StartIteration(t)
+		vu.StartIteration(t, k6test.WithIteration(0))
 
-		assert.True(t, browserRegistry.stopped.Load())
+		// Verify there are no browsers
+		assert.Equal(t, 0, browserRegistry.browserCount())
 	})
 
 	// This test ensures that the chromium browser's lifecycle is not controlled