Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork4.8k
FileLu - Fix memory leak during download and add multipart upload support#9024
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Conversation
ncw left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you identify the actual problem please?
The issue was caused by buffers not being released correctly when handling streamed responses.
The fix ensures that memory is freed promptly after each read operation, improving stability and preventing excessive RAM usage during large downloads.
That doesn't tell me what the problem is. Go has a garbage collector.
Once you've identified the actual problem can you send me a fix tojust fix that please? This patch rewrites the whole function changing everything for no good reason. It also has removed the pacer retries which are essential for reliability.
I don't mind looking at code that people have made with AI but I expect them to review it and tidy it first. Definitely remove the1)2)3) from the comments - they get out of date very quickly.
Maybe rewrite the whole function using the rest library which can do most of the work for you. Here is theOpen function from thebox backend which deals with range requests etc and should just work for you with minimal adaptation.
func (o*Object)Open(ctx context.Context,options...fs.OpenOption) (in io.ReadCloser,errerror) {ifo.id=="" {returnnil,errors.New("can't download - no id")}fs.FixRangeOption(options,o.size)varresp*http.Responseopts:= rest.Opts{Method:"GET",Path:"/files/"+o.id+"/content",Options:options,}err=o.fs.pacer.Call(func() (bool,error) {resp,err=o.fs.srv.Call(ctx,&opts)returnshouldRetry(ctx,resp,err)})iferr!=nil {returnnil,err}returnresp.Body,err}
Thank you
backend/filelu/filelu_object.go Outdated
| currentContents=currentContents[:count] | ||
| // 6) Validate response codes | ||
| ifresp.StatusCode!=http.StatusOK&&resp.StatusCode!=http.StatusPartialContent { | ||
| body,_:=io.ReadAll(resp.Body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is a memory leak waiting to happen here - loading the whole reponse into memory is not a good idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you for identifying the issue. I’ve updated the code and kept the pacer.call, only fixing the io.ReadAll.
Support uploading large files using multipart
Removed redundant error checks and updated response body closure.
kingston125 commentedDec 17, 2025
Hi@ncw |
Uh oh!
There was an error while loading.Please reload this page.
What is the purpose of this change?
This update resolves a memory leak caused by reading entire file downloads into memory (body, _ := io.ReadAll(resp.Body)).
It also adds support for uploading large files using multipart upload.
Was the change discussed in an issue or in the forum before?
No
Checklist