fix memory leak in gif thumbnail encoding #565
Conversation
When using an io.Pipe, it must be ensured that the pipe is drained. If it is not, all references required for the goroutine will not be freed by the garbage collector, as an active reference still exists (the abandoned goroutine the pipe is written to. To fix this, write to a non-blocking buffer. This may increase the RAM usage in the short run, but can be fully garbage collected, even if not read from. Since the size of the buffer is at most the size of the thumbnail and it being freed quickly, this should post a significantly smaller burden on the server. Signed-off-by: Moritz Poldrack <[email protected]> Fixes: 4378a4e ("Avoid use of buffers throughout thumbnailing") Fixes: t2bot#561
|
Since this is technically a DoS vulnerability, I just took the liberty of calculating its score.
|
|
This has been running on my server since the patch was submitted and there have been zero OOM kills since (I have limited MMR to 2GB of RAM for testing purposes). |
| _ = pw.Close() | ||
| } | ||
| }(pw, targetImg) | ||
| buf := bytes.NewBuffer(make([]byte, 0, 128*1024)) |
There was a problem hiding this comment.
these allocations are deliberately avoided to reduce memory volatility on large instances. This is the reason for the complicated io.Pipe stuff - if we can get that to work instead, would be best.
There was a problem hiding this comment.
That would require conditionally draining the pipe, which would probably introduce a fair amount of complexity, I will give it a look though. I would argue that even 50 MB of temporary allocation is preferable over a 10 MB memory leak though. This stuff adds up quickly. After running this, the normal ram usage of MMR is about 8-10 MB
There was a problem hiding this comment.
The average supported MMR deployment uses around 1gb of memory already, and is generally handling quite a few requests for thumbnails. 50mb per frame (or 50mb per thumbnail when animation is disabled) is quite a lot when considering traffic levels and performance impacts of the garbage collector.
MMR used to have "sawtooth" memory usage because of temporary buffers like this, and it was a major operational problem for hosting providers. To fix it, all code dropped in-memory buffers in favour of direct pipes or file system caching. This not only stabilized memory usage, but increased performance and throughput as well, though the underlying libraries for things like files still use a partial buffer themselves. This leads to a tiny bit of sawtooth-shaped graphs, but not nearly as bad as it could be with manual buffers.
When using an io.Pipe, it must be ensured that the pipe is drained. If it is not, all references required for the goroutine will not be freed by the garbage collector, as an active reference still exists (the abandoned goroutine the pipe is written to.
To fix this, write to a non-blocking buffer. This may increase the RAM usage in the short run, but can be fully garbage collected, even if not read from. Since the size of the buffer is at most the size of the thumbnail and it being freed quickly, this should post a significantly smaller burden on the server.
Fixes: 4378a4e ("Avoid use of buffers throughout thumbnailing")
Fixes: #561