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

io: MultiReader doesn't free input readers after use (Go 1.7) #16983

Closed
PhilipBorgesen opened this issue Sep 3, 2016 · 6 comments
Closed

io: MultiReader doesn't free input readers after use (Go 1.7) #16983

PhilipBorgesen opened this issue Sep 3, 2016 · 6 comments

Comments

@PhilipBorgesen
Copy link
Contributor

I've noticed that io.MultiReader doesn't set input reader references to nil before shrinking its slice. From my understanding of Go's garbage collector this means that the Readers won't be freed before the whole underlying array is freed.

In other words I propose that:

n, err = mr.readers[0].Read(p)
if err == EOF {
    mr.readers = mr.readers[1:]
}

is changed to:

n, err = mr.readers[0].Read(p)
if err == EOF {
    mr.readers[0] = nil
    mr.readers = mr.readers[1:]
}

In my encounter with this issue, the MultiReader may unnecessarily hold references to potentially large buffers (8 MB) for extended periods of time, wasting memory.

@randall77
Copy link
Contributor

@bradfitz

@rakyll
Copy link
Contributor

rakyll commented Sep 3, 2016

There is something more troubling about MultiReader. Even when MultiReader is collected, the readers are not. See https://play.golang.org/p/9dX8bk8P_I.

@rakyll
Copy link
Contributor

rakyll commented Sep 3, 2016

I put the example above in a loop and I now can see r1s are collected later in the program's life. It is just not immediately collected in the first passes. Maybe because of the mr->r1 dependency.

@gopherbot
Copy link

CL https://golang.org/cl/28533 mentions this issue.

@bradfitz
Copy link
Contributor

bradfitz commented Sep 6, 2016

@rakyll, more discussion of the liveness issues in #16996.

@gopherbot
Copy link

CL https://golang.org/cl/28771 mentions this issue.

gopherbot pushed a commit that referenced this issue Sep 8, 2016
Updates #16983
Updates #16996

Change-Id: I76390766385b2668632c95e172b2d243d7f66651
Reviewed-on: https://go-review.googlesource.com/28771
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@golang golang locked and limited conversation to collaborators Sep 8, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants