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

fix: Register processes for cleanup #153

Merged
merged 17 commits into from
Jun 25, 2024
Merged

fix: Register processes for cleanup #153

merged 17 commits into from
Jun 25, 2024

Conversation

zerok
Copy link
Contributor

@zerok zerok commented Jun 3, 2024

The idea here is to register k6 TestRuns, for which we do not want to wait for a result, with a cleanup routine so that no zombie processes stay around.

Right now the number of handler go-routines (those that wait for the TestRun processes) are limited in number. If that limit is reached, then the HTTP handler becomes blocking.

Tasks

Resolves #152

@zerok zerok force-pushed the zerok/fix-zombie-processes branch 2 times, most recently from 5fe6756 to 6f1dc5b Compare June 4, 2024 06:01
@zerok zerok marked this pull request as ready for review June 4, 2024 08:59
@zerok zerok requested review from julienduchesne and a team as code owners June 4, 2024 08:59
Copy link
Member

@iainlane iainlane left a comment

Choose a reason for hiding this comment

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

nice find!

main thing i'm wondering: what would happen if we didn't have the limit and instead updated the code to always Wait() but still allow any number of children to be launched? it'd remove the need for the pool. but perhaps it's not advisable ...

left comments with not doing ☝️ in mind 🙂

pkg/handlers/launch.go Outdated Show resolved Hide resolved
pkg/handlers/launch.go Outdated Show resolved Hide resolved
pkg/handlers/launch.go Outdated Show resolved Hide resolved
pkg/handlers/launch.go Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
pkg/handlers/launch.go Show resolved Hide resolved
@zerok zerok force-pushed the zerok/fix-zombie-processes branch from a11ccba to 1decf46 Compare June 4, 2024 16:35
Copy link
Member

@iainlane iainlane left a comment

Choose a reason for hiding this comment

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

alright! thanks for the updates. I'm scared about using more goroutines but the best thing to do might be to try it out

just had a couple of comments on the 429

pkg/handlers/launch.go Outdated Show resolved Hide resolved
pkg/handlers/launch.go Outdated Show resolved Hide resolved
pkg/handlers/launch.go Show resolved Hide resolved
pkg/handlers/launch.go Outdated Show resolved Hide resolved
Copy link
Member

@iainlane iainlane left a comment

Choose a reason for hiding this comment

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

let's give it a go 👍

@zerok zerok merged commit 127f1b9 into main Jun 25, 2024
4 checks passed
@zerok zerok deleted the zerok/fix-zombie-processes branch June 25, 2024 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle spawned child processes
3 participants