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 Goroutine leak in v2/command/formatter #10192

Merged
merged 2 commits into from Jan 23, 2023

Conversation

AhmedGrati
Copy link
Contributor

@AhmedGrati AhmedGrati commented Jan 22, 2023

What I did
In this PR, I fixed the Goroutine leak in the v2/command/formatter init function. The problem that we had previously, is known as The Forgotten Sender.
To fix the issue, we should simply quit the infinite loop in the Goroutine, by sending a signal that indicates that the main thread finished its execution. It's done using defer() and the quitChan channel.
A test of the Goroutine leak was added to the unit tests to make sure that the fix is working properly.

Related issue

Fixes #10157.

@ndeloof
Copy link
Contributor

ndeloof commented Jan 22, 2023

AFAICT, doing so after all rainbow colors have been used once, next call will block - which is not what we expect, first color should be re-used - so the loop

@AhmedGrati
Copy link
Contributor Author

Oh makes sense, didn't know about this constraint! I will think of a different solution.

Signed-off-by: AhmedGrati <ahmedgrati1999@gmail.com>
@AhmedGrati
Copy link
Contributor Author

Done @ndeloof.

Signed-off-by: AhmedGrati <ahmedgrati1999@gmail.com>
Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

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

LGTM

@glours glours enabled auto-merge January 23, 2023 10:20
@AhmedGrati
Copy link
Contributor Author

Cannot reproduce the failed e2e tests.

go test  -race -coverprofile=coverage.out -covermode=atomic -count=1 ./pkg/e2e -run ^TestCopy$
ok      github.com/docker/compose/v2/pkg/e2e    5.467s  coverage: 39.9% of statements

go test  -race -coverprofile=coverage.out -covermode=atomic -count=1 ./pkg/e2e -run ^TestLocalComposeRun$
ok      github.com/docker/compose/v2/pkg/e2e    13.272s coverage: 40.5% of statements

The error in the pipeline is really weird.

Stderr:   unknown shorthand flag: 'f' in -f

Any help would be much appreciated.

@ndeloof
Copy link
Contributor

ndeloof commented Jan 24, 2023

I had to revert this PR, has this breaks compose up use of color assignment

@AhmedGrati
Copy link
Contributor Author

@ndeloof Can you give me a scenario that helps me to reproduce the error, please?

@ndeloof
Copy link
Contributor

ndeloof commented Jan 24, 2023

plain good old docker compose up was broken: as defer runs after init completion, the color loop hangs and we never get logs.
cleanup should take place after pluginMain execution

@AhmedGrati
Copy link
Contributor Author

AhmedGrati commented Jan 24, 2023

Thanks @ndeloof, I'll fix it and open a new PR.

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.

[BUG] goroutine leak in init() of v2/cmd/formatter ?
4 participants