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

mime/multipart: TempFile file hangs around on disk after usage in multipart/formdata.go #20253

Open
nilslice opened this issue May 5, 2017 · 15 comments
Labels
help wanted NeedsFix
Milestone

Comments

@nilslice
Copy link

@nilslice nilslice commented May 5, 2017

Hey Go team,

I'd like to know if this is a bug or expected behavior. If it is a bug, I would like to write a patch to fix it.


Please answer these questions before submitting your issue. Thanks!

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

go version go1.8 linux/amd64

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

GOOS="linux"
GOARCH="amd64"

What did you do?

When uploading a file to a Go HTTP server, the call to req.ParseMultipartForm() subsequently calls readForm() and if the file size exceeds the maxMemory setting it creates a file on disk using the ioutil.TempFile() func, but the resulting file is never removed by the Go program. Unix behavior aside, based on the documentation for ioutil.TempFile(), the caller should remove the file once it is done using it. The file is only ever removed from the Go program if the io.Copy returns a non-nil error.

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.

I would need to show a lot of code to demonstrate this, but you can see in the Go src here how the file is never removed: https://github.com/golang/go/blob/master/src/mime/multipart/formdata.go#L78

A link on play.golang.org is best.

What did you expect to see?

I expect that the temp file is removed after the file is copied to it's designated location on disk.

What did you see instead?

my /tmp directory is littered with multipart-XXXXXXXX files

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented May 5, 2017

When you're done with the multipart files, you can call:
https://golang.org/pkg/mime/multipart/#Form.RemoveAll

... which is accessible here:
https://golang.org/pkg/net/http/#Request.MultipartForm

But, yeah, maybe this could be more automatic. Maybe the multipart.File's concrete type could delete-on-Close.

Feel free to work on it!

@bradfitz bradfitz changed the title ioutil.TempFile file hangs around on disk after usage in multipart/formdata.go mime/multipart: TempFile file hangs around on disk after usage in multipart/formdata.go May 5, 2017
@bradfitz bradfitz added this to the Go1.10 milestone May 5, 2017
@nilslice
Copy link
Author

@nilslice nilslice commented May 5, 2017

Thanks for the suggestions! I wasn't aware of Form.RemoveAll - good to know.

From my perspective, the complete process of dealing with HTML forms / other HTTP clients uploading files to my server shouldn't involve removing the temp files w/ RemoveAll. Based on the UX of a typical Go programmer, automatically cleaning up those files doesn't feel overly magical to me.

I'll work on it and submit if something clicks.

@kennygrant
Copy link
Contributor

@kennygrant kennygrant commented Nov 3, 2017

@nilslice still working on this?

We were hit by a similar problem (have small servers accepting file uploads, and tmp files filled the disk at times if not flushed regularly), and I too was unaware of Form.RemoveAll, but delete on close would be even better.

Happy to look at it if you're busy or leave it to you if you'd like to look at it.

@nilslice
Copy link
Author

@nilslice nilslice commented Nov 3, 2017

@kennygrant - I have a partial fix, but have been swamped with other work. If it's imperative to your project that it's patched, go for it! Otherwise, I'd still like to finish it. But no problem whatsoever if you want to get the patch in.

@kennygrant
Copy link
Contributor

@kennygrant kennygrant commented Nov 3, 2017

That's ok, I'll leave it to you if you have started. We have a workaround for now.

@rsc rsc removed this from the Go1.10 milestone Nov 22, 2017
@rsc rsc added this to the Go1.11 milestone Nov 22, 2017
@Kagami
Copy link

@Kagami Kagami commented Mar 17, 2018

@nilslice
Copy link
Author

@nilslice nilslice commented Mar 17, 2018

@Kagami - it does appear to make the call, but the last time I tested it the files were still on disk.

The blame shows that the code has been in the tree for ~7 years, well before this issue was originally filed: https://github.com/golang/go/blame/678dede7bc22a7cccfe78c42711478a9b1ecf4d3/src/net/http/server.go#L1553-L1555

Have you tried this to verify that the files are cleaned up after the request?

I'm trying to get my CL out this month or early next (taking a couple days off from work to do long put-off open source tasks I have queued up!).

@andybons andybons added the NeedsFix label Mar 19, 2018
@gopherbot
Copy link

@gopherbot gopherbot commented May 14, 2018

Change https://golang.org/cl/113055 mentions this issue: mime/multipart: properly remove temp files in case of error

@lpar
Copy link

@lpar lpar commented Aug 14, 2018

If this won't be fully fixed in 1.10, how about mentioning the need to call RemoveAll in the documentation for ParseMultipartForm?

@dvdscripter
Copy link

@dvdscripter dvdscripter commented Aug 21, 2018

I'm no expert but you should be able to remove files without closing them?

...
file, _, err := r.FormFile("file")
if err != nil {
	return err
}
defer file.Close()
...

Leaves no "multipart*" files behind.

@agnivade
Copy link
Contributor

@agnivade agnivade commented Dec 19, 2018

@nilslice - Did you get a chance to work on this ? As per @Kagami's comment, it seems like the file is indeed removed.

Please also try with the 1.12beta1 to see if there has been any changes.

@nilslice
Copy link
Author

@nilslice nilslice commented Dec 19, 2018

@agnivade - I have not recently had time to wrap this up. If anyone would like to take it, or if it is indeed no longer an issue, that's awesome 😄

@chris3torek
Copy link

@chris3torek chris3torek commented Apr 1, 2020

Note that this is still a problem with code generated by swagger. It's hard to work around in all cases. Success cases are easy enough, as your swagger parameters include the original HTTP request so you can do:

form := params.HTTPRequest.MultipartForm
err = form.RemoveAll()
// handle err, if you like
form.File = nil // in case the bug is fixed in the future

However, this doesn't handle every possible error path (specifically any where your own handler never gets called due to an error detected inside swagger itself).

@tooptoop4
Copy link

@tooptoop4 tooptoop4 commented Oct 28, 2020

@bradfitz where is remove in net/http/#Request.MultipartForm ?

@dvdscripter
Copy link

@dvdscripter dvdscripter commented Oct 29, 2021

Can someone validate in another OS? https://play.golang.org/p/5n60s8qSf_k

  • go1.17.1 windows/amd64: file hanging
  • go1.17.1 linux/amd64: clear!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsFix
Projects
None yet
Development

No branches or pull requests