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

*: always call Close() on http.Response.Body #75136

Merged
merged 1 commit into from Jan 26, 2022

Conversation

knz
Copy link
Contributor

@knz knz commented Jan 19, 2022

Since go 1.17.6, forgetting to call Close() results in goroutine
leaks.

See: golang/go#50652

Release note: None

@knz knz requested a review from a team January 19, 2022 08:35
@knz knz requested review from a team as code owners January 19, 2022 08:35
@knz knz requested review from stevendanna and removed request for a team January 19, 2022 08:35
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz added this to In progress in DB Server & Security via automation Jan 19, 2022
@knz knz requested a review from a team January 19, 2022 08:36
Since go 1.17.6, forgetting to call Close() results in goroutine
leaks.

Release note: None
Copy link
Contributor

@Azhng Azhng left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz and @stevendanna)


pkg/server/server_test.go, line 272 at r1 (raw file):

According to:

If the returned error is nil, the Response will contain a non-nil Body which the user is expected to close.

Should we only close the body after checking err is nil below?

Copy link
Contributor

@Azhng Azhng left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @stevendanna)


pkg/server/server_test.go, line 272 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

According to:

If the returned error is nil, the Response will contain a non-nil Body which the user is expected to close.

Should we only close the body after checking err is nil below?

Actually nvm, the err comes from the io.Copy

@knz
Copy link
Contributor Author

knz commented Jan 25, 2022

TFYR!

bors r=Azhng

@craig
Copy link
Contributor

craig bot commented Jan 25, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jan 26, 2022

Build succeeded:

@craig craig bot merged commit f8a330b into cockroachdb:master Jan 26, 2022
DB Server & Security automation moved this from In progress to Done 21.2 Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
DB Server & Security
  
Done 21.2
Development

Successfully merging this pull request may close these issues.

None yet

3 participants