Skip to content

Conversation

seiyab
Copy link
Member

@seiyab seiyab commented Mar 17, 2024

  • Updated Unreleased section in CHANGELOG or it's not notable changes.
    • It's not a notable change.

Hi, I found closers that aren't closed in doghouse. Since it's minor fix, feel free to ignore this.

Go Doc comment on datastore.NewClient says:

// Call (*Client).Close() when done with the client.

Implementation of (*Client).Close() looks to close connection pooling.

// Close closes the Client. Call Close to clean up resources when done with the
// Client.
func (c *Client) Close() error {
	return c.connPool.Close()
}

So it looks better to be closed.

Adding context, I'm not familiar doghouse at all (though I'm interested in reviewdog). Even I don't know how to run doghouse (#1194). I'm developing a static code analyzer and randomly scanning open projects to find practical false positives so that I can improve recall of the analyzer. I just found this problem by the analyzer and consider it's a true positive.

Copy link
Member

@haya14busa haya14busa left a comment

Choose a reason for hiding this comment

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

LGTM.
Thank you for your contribution 👍

@haya14busa haya14busa merged commit d694c56 into reviewdog:master Apr 8, 2024
@seiyab seiyab deleted the close-datstore-client branch April 8, 2024 13:39
@seiyab
Copy link
Member Author

seiyab commented Apr 8, 2024

@haya14busa
Thank you for merging.

I've also found a (possibly security) issue. Where can we discuss?

@seiyab
Copy link
Member Author

seiyab commented Apr 10, 2024

@reviewdog/maintainers
ping. #1692 (comment)
I consider that I shouldn't submit public PR that might relates to a vulnerability.

@shogo82148
Copy link
Contributor

@haya14busa
Copy link
Member

I just enabled private vulnerability reporting.
@seiyab can you report it?

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.

3 participants