Skip to content
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

Prevent goroutines getting blocked in js tests #2284

Merged
merged 1 commit into from
Dec 13, 2021

Conversation

mstoykov
Copy link
Contributor

@mstoykov mstoykov commented Dec 3, 2021

The predominant reason was the use of context.Background() for a context, which leads to a goroutine waiting on it ending, to block indefinitely.

The other case were gRPC tests which did not close their connections.

@mstoykov mstoykov added the tests label Dec 3, 2021
@github-actions github-actions bot requested review from na-- and yorugac December 3, 2021 14:39
The predominant reason was the use of context.Background() for
a context, which leads to a goroutine waiting on it ending, to block
indefinitely.

The other case were gRPC tests which did *not* close their connections.
@mstoykov mstoykov force-pushed the fixStrandledGoroutines branch from 25477f3 to 3d2d2dc Compare December 3, 2021 14:40
@mstoykov
Copy link
Contributor Author

mstoykov commented Dec 3, 2021

These were find around #2228 (comment) and were mostly caught by paniccing when context.Done() return nil at

k6/js/runner.go

Line 652 in 5c14dae

<-ctx.Done()
- I am not really certain if it won't be good idea to check this in the production code as well 🤔

Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

  • I am not really certain if it won't be good idea to check this in the production code as well thinking

If this would happen in production then I expect to see a lot of stuff stuck then it should be easy to catch, so I'm not sure. Considering the numbers of VUs we have, I see it relevant if there is a case where it could happen just for some VUs. But I don't see/know that case.

@yorugac yorugac removed their request for review December 10, 2021 15:57
@mstoykov mstoykov merged commit 8f5bafe into master Dec 13, 2021
@mstoykov mstoykov deleted the fixStrandledGoroutines branch December 13, 2021 08:31
@mstoykov
Copy link
Contributor Author

@codebien I doubt we have a bug in the production code, I was wondering about guarding again the same regression in test code, but this also didn't catch some variants and no other test broke in the whole codebase so 🤷

Let's leave it for now

@na-- na-- added this to the v0.36.0 milestone Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants