NotificationsYou must be signed in to change notification settings
Fork928
Star10.1k

fix(scaletest/workspacetraffic): wait for non-zero metrics before cancelling in TestRun#9663

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

johnstcn merged 3 commits intomainfromcj/flake-workspacetraffic-testrun-pty

Sep 13, 2023

Merged

fix(scaletest/workspacetraffic): wait for non-zero metrics before cancelling in TestRun#9663

johnstcn merged 3 commits intomainfromcj/flake-workspacetraffic-testrun-pty

Sep 13, 2023

Conversation

Copy link

Member

johnstcn commentedSep 13, 2023•
edited
Loading

Hopefullyfixes#9661

fix(scaletest/workspacetraffic): TestRun: wait for non-zero metrics b…

fdd76b9

…efore cancelling

github-actionsbot assignedjohnstcn

Sep 13, 2023

johnstcn changed the title~~fix(scaletest/workspacetraffic): TestRun: wait for non-zero metrics before cancelling~~fix(scaletest/workspacetraffic): wait for non-zero metrics before cancelling in TestRun

Sep 13, 2023

johnstcn marked this pull request as ready for review

September 13, 2023 14:03

johnstcn requested review frommafredri,spikecurtis andmtojek

September 13, 2023 14:03

mtojek approved these changes

Sep 13, 2023

View reviewed changes

Copy link

Member

mtojek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Fingers crossed!

scaletest/workspacetraffic/run_test.go

		require.NoError(t, runner.Run(ctx, "", &logs), "unexpected error calling Run()")

		gotMetrics := make(chan struct{})
		go func() {

Copy link

Member

mtojekSep 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

nit: I'm always confused if there is a possibility of a routine leak. defer goes first then routine is killed, right?

Copy link

MemberAuthor

johnstcnSep 13, 2023•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Defer runs when the surrounding function returns. Soassert.Eventually() has to finish beforegotMetrics is closed, and<-gotMetrics will block until that happens. Then we cancel the context passed torunner.Run, which will cause it to exit, then we closerunDone, which unblocked<-runDone.

I thinkgoleak would have caught a goroutine leak here as well.