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.

Sign up for GitHub

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

Jump to bottom

Remove detected goroutine leaks #2833

Merged
merged 4 commits into from
Mar 9, 2023
Merged

Remove detected goroutine leaks #2833

merged 4 commits into from
Mar 9, 2023

Conversation

codebien
Copy link
Contributor

No description provided.

@codebien codebien self-assigned this Dec 22, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 22, 2022 •
edited
Loading

Codecov Report

Merging #2833 (8cc0c9e) into master (367163b) will increase coverage by 0.07%.
The diff coverage is 83.48%.

❗ Current head 8cc0c9e differs from pull request most recent head 6ce8ddf. Consider uploading reports for the commit 6ce8ddf to get more accurate results

@@            Coverage Diff             @@
##           master    #2833      +/-   ##
==========================================
+ Coverage   76.84%   76.92%   +0.07%     
==========================================
  Files         229      229              
  Lines       16924    16946      +22     
==========================================
+ Hits        13005    13035      +30     
+ Misses       3077     3074       -3     
+ Partials      842      837       -5
Flag Coverage Δ
ubuntu 76.84% <83.48%> (+0.08%) ⬆️
windows 76.75% <83.48%> (+0.10%) ⬆️

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

Impacted Files Coverage Δ
cmd/tests/tests.go 46.15% <0.00%> (+1.70%) ⬆️
log/loki.go 30.70% <0.00%> (-6.36%) ⬇️
log/file.go 79.66% <50.00%> (-1.59%) ⬇️
lib/testutils/httpmultibin/httpmultibin.go 92.64% <89.18%> (+2.69%) ⬆️
cmd/root.go 95.33% <94.54%> (+13.51%) ⬆️
js/initcontext.go 80.00% <0.00%> (-4.00%) ⬇️
lib/executor/vu_handle.go 93.45% <0.00%> (-0.94%) ⬇️
api/v1/metric.go 77.27% <0.00%> (+9.09%) ⬆️

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

na--
na-- previously approved these changes Dec 23, 2022
View reviewed changes
Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

❤️

@codebien codebien force-pushed the no-go-leaks branch 5 times, most recently from 6c51141 to 5b191f4 Compare December 23, 2022 15:42
@na-- na-- added this to the v0.43.0 milestone Jan 3, 2023
@na-- na-- mentioned this pull request Jan 10, 2023
Merged
@codebien codebien changed the base branch from remove-the-engine to master January 13, 2023 14:16
@codebien codebien dismissed na--’s stale review January 13, 2023 14:16

The base branch was changed.

na--
na-- reviewed Jan 13, 2023
View reviewed changes
cmd/root.go Outdated Show resolved Hide resolved
@codebien codebien force-pushed the no-go-leaks branch 2 times, most recently from c0d84e0 to be16867 Compare January 17, 2023 15:17
@codebien codebien marked this pull request as ready for review January 17, 2023 16:59
@github-actions github-actions bot requested review from imiric and na-- January 17, 2023 16:59
@codebien codebien force-pushed the no-go-leaks branch 4 times, most recently from e7b6db8 to 1e60b4f Compare January 18, 2023 11:43
@codebien
Copy link
Contributor Author

Rebased after the e2e tests refactor

codebien
codebien commented Jan 18, 2023
View reviewed changes
cmd/root.go Outdated Show resolved Hide resolved
imiric
imiric reviewed Jan 20, 2023
View reviewed changes
Copy link
Contributor

@imiric imiric left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It mostly LGTM, just a few non-blocking comments and questions.

@codebien How did you see the goleak report? I couldn't see anything from go test -v -race -count=1 ./cmd/tests on master, and I can't figure it out from their README 😕 I would like to see leaks detected on master and then fixed on this branch.

lib/testutils/httpmultibin/httpmultibin.go Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
@codebien
Copy link
Contributor Author

Hi @imiric, thanks for your review.

On master, the leak is not reported because we are skipping the check. You should see the report removing this option.

k6/cmd/tests/tests.go

Lines 55 to 57 in 0b563d4

// TODO: figure out why logrus' `Entry.WriterLevel` goroutine sticks
// around and remove this exception.
opt := goleak.IgnoreTopFunction("io.(*pipe).read")

@codebien
Copy link
Contributor Author

codebien commented Jan 25, 2023 •
edited
Loading

Hey @imiric,
I tried to apply some fixes for your request changes, the current log handling code is not as clean as I would but I think to do something better we need to start creating a dedicated component for logging (#1939, #2287) then it should be moved to a dedicated PR.

Regarding Httpmultibin I've moved the ws-close as a dedicated handler with the expectation that the server closes the connection. Instead, the echo handler waits for the client to start the closing ops.
Some ws tests weren't aligned with this strict principle so I had to adjust how we are using them.

The current commit history is not optimal so if we have a consensus on the current design I will re-arrange for having atomic commits.

@na-- na-- modified the milestones: v0.43.0, v0.44.0 Feb 8, 2023
@codebien codebien force-pushed the no-go-leaks branch 2 times, most recently from c2e08fb to 64c9809 Compare February 21, 2023 18:59
imiric
imiric suggested changes Feb 24, 2023
View reviewed changes
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

Almost LGTM 😄

I'd only like to move asyncHook to the log package.

There are many subtle changes to the logging mechanism here, and while it visually seems safe, I hope we're not breaking some obscure behavior. The testing for this isn't great, so hopefully we can catch any issues in manual test runs. 🤞

lib/testutils/httpmultibin/httpmultibin.go Show resolved Hide resolved
log/file_test.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
na--
na-- reviewed Mar 6, 2023
View reviewed changes
cmd/tests/cmd_run_test.go Outdated Show resolved Hide resolved
@na--
Copy link
Member

na-- commented Mar 6, 2023

Generally LGTM, with a few nitpicks I commented on or 👍 already existing comments

@codebien codebien force-pushed the no-go-leaks branch 4 times, most recently from ba09a69 to 6df4ba0 Compare March 9, 2023 09:26
@codebien codebien requested a review from imiric March 9, 2023 09:42
@codebien codebien requested a review from na-- March 9, 2023 15:42
na--
na-- approved these changes Mar 9, 2023
View reviewed changes
Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

🤞

@codebien codebien merged commit 333a3d1 into master Mar 9, 2023
@codebien codebien deleted the no-go-leaks branch March 9, 2023 16:27
@na-- na-- mentioned this pull request Mar 31, 2023
Merged
mstoykov added a commit that referenced this pull request Jul 13, 2023
@mstoykov
Fix loki log output exiting before push is finished
a6fc5b8 
After #2833 log outputs were stopped but loki didn't wait until it push
was finished to signal it was done.

Which leads to the actual k6 process exiting before loki could flush its messages.
mstoykov added a commit that referenced this pull request Jul 13, 2023
@mstoykov
Fix loki log output exiting before push is finished
3eaf6f6 
After #2833 log outputs were stopped but loki didn't wait until it push
was finished to signal it was done.

Which leads to the actual k6 process exiting before loki could flush its messages.
@mstoykov mstoykov mentioned this pull request Jul 13, 2023
Merged
mstoykov added a commit that referenced this pull request Jul 13, 2023
@mstoykov
Fix loki log output exiting before push is finished
ac448b0 
After #2833 log outputs were stopped but loki didn't wait until it push
was finished to signal it was done.

Which leads to the actual k6 process exiting before loki could flush its messages.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mstoykov mstoykov mstoykov left review comments

@na-- na-- na-- approved these changes

@imiric imiric imiric approved these changes

Assignees

@codebien codebien

Labels
None yet
Projects
None yet
Milestone
v0.44.0
Development

Successfully merging this pull request may close these issues.
5 participants
@codebien @codecov-commenter @na-- @mstoykov @imiric