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

containeranalysis: close grpcClient #50

Merged
merged 4 commits into from Dec 15, 2021
Merged

Conversation

thepwagner
Copy link
Collaborator

In containeranalysis.NewClient(), we allocate a containeranalysis
client, but only use the .GetGrafeasClient() member.

This results in leaking the containeranalysis client's connection pool,
whenever a new client is created.

Longer term, we should look at reusing connections as an alternative
solution to this leak: there are other performance advantages.

Co-Authored-By: Brian Chen brian.chen@shopify.com
Co-Authored-By: Aline Shulzhenko lynnsh@users.noreply.github.com

Related

In `containeranalysis.NewClient()`, we allocate a containeranalysis
client, but only use the `.GetGrafeasClient()` member.

This results in leaking the containeranalysis client's connection pool,
whenever a new client is created.

Longer term, we should look at reusing connections as an alternative
solution to this leak: there are other performance advantages.

Co-Authored-By: Brian Chen <brian.chen@shopify.com>
Co-Authored-By: Aline Shulzhenko <lynnsh@users.noreply.github.com>
@thepwagner
Copy link
Collaborator Author

_lynnsh noted from https://pkg.go.dev/cloud.google.com/go/containeranalysis/apiv1#Client.GetGrafeasClient

Calling Close on either the grafeas or containeranalysis client will close the shared connection in both.

I think that comment is out of date, opened googleapis/google-cloud-go#5217 to confirm or deny this.

Copy link
Collaborator

@dani-santos-code dani-santos-code left a comment

Choose a reason for hiding this comment

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

cool! thanks for showing us how we can use pprofile to identify problems! this makes sense!

@thepwagner
Copy link
Collaborator Author

This is behaving great in our test environment, :shipit: and closing #20

@thepwagner thepwagner merged commit 3536639 into main Dec 15, 2021
@thepwagner thepwagner deleted the container-analysis-client branch December 15, 2021 16:34
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.

None yet

2 participants