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

compress/gzip: memory leak using gzip.Writer #58455

Closed
kitarp29 opened this issue Feb 10, 2023 · 5 comments
Closed

compress/gzip: memory leak using gzip.Writer #58455

kitarp29 opened this issue Feb 10, 2023 · 5 comments
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.

Comments

@kitarp29
Copy link

What version of Go are you using (go version)?

$go version
go version go1.19.3 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

GOHOSTOS="linux" which is on Ubuntu 20.04, also tried it on MacOS.

go env Output
$ go env
GO111MODULE="auto"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/kitarp29/.cache/go-build"
GOENV="/home/kitarp29/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/kitarp29/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/kitarp29/go/"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.19.3"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1140511453=/tmp/go-build -gno-record-gcc-switches"

What did you do?

This was an issue I ran into while at work this week. We are building a service on Golang and compressing the data before I save it to the DB. We tried using compress/gzip.

Implemented the basic way to compress the data using the NewWriter Function : Here. Problem is that when we tried this solution at scale(1 million entries at a time) there was a memory leak. We did Lfush and close all the writers and buffers we used. Still, it continued, verified using pprof.

We made an in-house implementation to solve the issue. We used new(gzip.Writer) to create a Writer for us.
Tried like the last implementation, but to our surprise, it did not compress the string at all. In fact, the compressed data was larger than the original!
There was no compilation error whatsoever. Reason it increased was as the file/data is compressed an extra footer and added is added. After going through the code of gzip: Here

I realized that when we use gzip.NewWriter() we are expected to pass an io.writer. This writer replaces the writer in the struct of gzip.Writer. I am assuming only then does it compresses the file.

Even I am not sure if it is BUG or not. I just wanted to share this and reduce the pain for someone else using this pkg.

What did you expect to see?

The pkg to compress the data provided. But it did not.

What did you see instead?

An uncompressed, even increased data than original
@seankhliao
Copy link
Member

In general, if there are constructors provided, it's expected for you to use them (unless the zero value has been explicitly documented to work).
If you have an reproducer that can demonstrate a leak in gzip.Writer, we'd like to see it.

@seankhliao seankhliao changed the title compress/gzip: Compression does not work with new(gzip.Writer) compress/gzip: memory leak using gzip.Writer Feb 10, 2023
@seankhliao seankhliao added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Feb 10, 2023
@rittneje
Copy link

We used new(gzip.Writer) to create a Writer for us. Tried like the last implementation, but to our surprise, it did not compress the string at all. In fact, the compressed data was larger than the original!

Given that (1) gzip.Writer.Write would panic if you did that, and (2) gzip.Writer does not expose any way to ask for the compressed data anyway, I don't understand what you did here.

@bcmills bcmills added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Feb 13, 2023
@gopherbot
Copy link

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

@kitarp29
Copy link
Author

I can't actually share the memory leak proof. As it was work related and running pprof on the code will entail more parts of the project.

One Major issue that I wanted to highlight. You can use new(gzip.Writer) instead of using the function gzip.NewWriter().

In the first case we don't provide a writer to the pkg. But it still complies.
It won't compress but it compiles and does run.

As I said I am not sure if it's a Bug or not
But definetly something needed to be mebtioned in the Docs.

@ianlancetaylor
Copy link
Contributor

@kitarp29 Thanks, but code like new(gzip.Writer) can't be prevented in Go. But it clearly can't work. I don't think we need to document that, when gzip.NewWriter is right there. That isn't something for package documentation, it's something more for a blog entry.

@golang golang locked and limited conversation to collaborators Apr 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@ianlancetaylor @bcmills @gopherbot @seankhliao @rittneje @kitarp29