Use temporary file to avoid out-of-memory when receiving big chunks. #216

typeless · 2016-11-22T11:35:26Z

Not perfect but I think it's a reasonable solution.
For small request bodies, I suppose performance wouldn't be an issue.
For large ones, this seems to be a necessary evil.

Fixes #218

Not perfect but I think it's a reasonable solution. For small request bodies, I suppose performance wouldn't be an issue. For large ones, this seems to be a necessary evil.

typeless · 2016-11-22T11:38:54Z

Any open issue for this?

strk · 2016-11-22T11:46:45Z

routers/repo/http.go


 	if h.cfg.OnSucceed != nil {
-		input, err = ioutil.ReadAll(reqBody)
+		tmpfile, err := ioutil.TempFile("", "gogs")


I hadn't found info about the permissions that TempFile would put on the file, I guess it should be checked

https://github.com/golang/go/blob/master/src/io/ioutil/tempfile.go#L55

Is this what you are looking for?

codecov-io · 2016-11-22T11:47:43Z

Current coverage is 3.03% (diff: 100%)

Merging #216 into master will not change coverage

@@            master      #216   diff @@
========================================
  Files           33        33          
  Lines         8106      8106          
  Methods          0         0          
  Messages         0         0          
  Branches         0         0          
========================================
  Hits           246       246          
  Misses        7840      7840          
  Partials        20        20

Powered by Codecov. Last update cb16028...a0ca633

strk · 2016-11-22T11:51:42Z

No issue that I know of, please leave here a reference to the Gogs PR which in turn has more refs to Gogs issue and if you feel like it, also add a local issue

lunny · 2016-11-22T13:45:35Z

I don't think it's a good idea to use temporary file always since most time, the upload data is small data on git situation. For large files, lfs maybe a better choice and we have a merge-ready PR #122 .

typeless · 2016-11-22T14:39:18Z

@lunny I am genuinely open to a better solution but I am afraid that LFS cannot solve the problem if the repository in huge size consists of many small files rather than some big binary blobs.

Maybe someone else has experiences about how other similar projects deal with such issue?

lunny · 2016-11-22T16:14:33Z

I think this PR maybe resolve #218 . Could we chose memory or file according the content-size ?

thibaultmeyer · 2016-11-22T16:22:48Z

I think a max size must be defined (maybe on app.ini) to avoid app crash or other kind of strange issue if free space is not enough to store temp data.

tboerger · 2016-11-22T16:24:00Z

The current approach will slow down all writes, and that's sad

lunny · 2016-11-22T16:25:13Z

^
That's what I want to say. And most times, it will slow down.

thibaultmeyer · 2016-11-22T16:26:31Z

Id not possible to process incoming data as stream rather than just wait upload was completed ?

lunny · 2016-11-22T16:26:33Z

Maybe we can use this library to do something https://github.com/edsrzf/mmap-go

strk · 2016-11-22T16:30:47Z

A stream oriented approach would be best, but it isn't trivial to implement (me and @bkcsoft tried it with no much luck) - if I recall correctly it had to do with the way the code was written, that's to say a refactoring might make it easier to do stream-oriented. Want to give it another try ?

lunny · 2016-11-22T16:36:55Z

I think mmap is the correct method to resolve it. Keep the performance and resolve the large file bug.

typeless · 2016-11-23T05:40:20Z

If Windows support is not an issue then open(2) with O_TMPFILE could be an option as well. ( I am not sure how well the status of cross-platform support of Gitea is)

tboerger · 2016-11-23T06:09:04Z

Gitea MUST work on Windows as well

typeless · 2016-11-23T06:15:06Z

@tboerger If that's the case, then O_TMPFILE and mmap would introduce some extra platform-dependent complexities.

strk · 2016-11-23T07:03:56Z

I'm replying to this comment which I don't find on the web ui
anymore (deleted?):

On Tue, Nov 22, 2016 at 06:12:49PM -0800, typeless wrote:

Just took a glance and it seems a stream-oriented approach would be more probable if we can change the function signature of the [OnSucceed](https://github.com/go-gitea/gitea/blob/master/routers/repo/http.go#L233) from func(rpc string, input []byte) to func(rpc string, input io.Reader).

This kind of refactoring did sound like a good idea, did you
hit a roadblock that made you remove the comment ?

typeless · 2016-11-23T07:36:38Z

@strk Yeah, I did delete the comment right after I realized that it's not just simply changing the function signature. The problem is that we have to read though the stream twice and the reader is not necessarily seekable.

Need to take a look at the underlying mechanism of the origin of the request body (how the data is connected/copied to the io.Reader).

lunny · 2016-11-23T08:19:27Z

This is only because we maybe called h.cfg.OnSucceed. Or we can just let cmd.Stdin = reqBody.

strk · 2016-11-23T08:59:05Z

Maybe there's the possibility of using a Tee

typeless · 2016-11-23T09:25:57Z

This is only because we maybe called h.cfg.OnSucceed. Or we can just let cmd.Stdin = reqBody.

@lunny Yeah, hopefully someone knows about why it has to be a two-pass operation in the first place.

typeless · 2016-11-23T09:33:55Z

Maybe there's the possibility of using a Tee

@strk io.Pipe() + goroutines seems to be the straightforward way. But I guess that would make the Read of Tee blocking if the other end of read in OnSucceed isn't running simultaneously.

bkcsoft · 2016-11-23T16:26:55Z

How about this then: (AFAIK) you can check the payload-size and for anything above MAX_HTTP_PAYLOAD would go into a TempFile ?

bkcsoft · 2016-11-23T16:30:05Z

About using TeeWriter, I've tried that without success... The good fix for this is to move h.Cfg.OnSucced to .git/hooks/post-recieve (where it belongs), but doing that would make said git-hook unavailable to the User/Admin (i.e. break backwards-compat).

typeless · 2016-11-24T02:37:19Z

@bkcsoft I have a PR which adds what you suggested but the value of MAX_HTTP_PAYLOAD is still a TBD.
Any suggestions on the value and the location of the constant definition?

I have lost the original repo during the process of transitioning the Github fork to Gitea from Gogs (Local repo is still there, though I am afraid it's impossible to restore the tracking for Github). I would have to either resend this PR or send another PR addressing the threshold after this getting merged.

Which means, I will probably leave the PR as-is and send follow-ups for MAX_HTTP_PAYLOAD if that's the agreed direction.

strk · 2016-11-24T07:58:29Z

if you're going to define a new constant use CamelCase please ..

tboerger · 2016-11-24T10:17:49Z

I have moved it to 1.1.0 for now.

strk · 2016-12-24T08:40:43Z

For the record, @unkwon merged this in Gogs: gogs/gogs#2960

lunny · 2016-12-24T08:50:31Z

A temporary method is to write to a template file in an another goroutine and write actions information to database. Every time write disk will slow the operations. So I don't think merge this directly is a good idea.

typeless · 2016-12-24T09:27:14Z

Is there any opportunity to handle this more gracefully (i.e. to avoid copying completely) if those operations are replaced with git library calls instead of fork/exec to git? Just saying.

lunny · 2016-12-24T09:34:39Z

I think @bkcsoft 's idea is the good direction.

About using TeeWriter, I've tried that without success... The good fix for this is to move h.Cfg.OnSucced to .git/hooks/post-recieve (where it belongs), but doing that would make said git-hook unavailable to the User/Admin (i.e. break backwards-compat).

bkcsoft · 2016-12-25T12:29:37Z

@lunny using post-recieve-hook would be 2 PRs in this case

move the user-provided hook somewhere else (hooks/gitea-post-recieve?) and have hooks/post-recieve call that script
move this code into hooks/post-recieve after user-provided script is ran

lunny · 2017-02-05T14:05:44Z

I think this should be closed. And let's discuss in #218

* Revert YAML-rendering of markdown files * tmp

typeless added 2 commits October 10, 2016 15:03

Use temporary file to avoid out-of-memory when receiving big chunk.

4cb89bc

Not perfect but I think it's a reasonable solution. For small request bodies, I suppose performance wouldn't be an issue. For large ones, this seems to be a necessary evil.

Must close the open file to avoid fd leaks

a0ca633

typeless changed the title ~~Develop~~ Use temporary file to avoid out-of-memory when receiving big chunks. Nov 22, 2016

strk reviewed Nov 22, 2016

View reviewed changes

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 23, 2016

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 24, 2016

tboerger added this to the 1.1.0 milestone Nov 24, 2016

tboerger added the type/enhancement An improvement of existing functionality label Nov 24, 2016

lunny closed this Feb 5, 2017

tboerger added reviewed/invalid and removed type/enhancement An improvement of existing functionality lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 10, 2017

tboerger removed this from the 1.1.0 milestone Feb 10, 2017

ethantkoenig added a commit to ethantkoenig/gitea that referenced this pull request Sep 18, 2017

Revert YAML-rendering of markdown files (go-gitea#216)

cec2a40

* Revert YAML-rendering of markdown files * tmp

go-gitea locked and limited conversation to collaborators Nov 23, 2020

delvh added issue/not-a-bug The reported issue is the intended behavior or the problem is not inside Gitea and removed reviewed/invalid labels Oct 7, 2023

Uh oh!

Use temporary file to avoid out-of-memory when receiving big chunks. #216

Use temporary file to avoid out-of-memory when receiving big chunks. #216

Uh oh!

Conversation

typeless commented Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typeless commented Nov 22, 2016

Uh oh!

strk Nov 22, 2016

Choose a reason for hiding this comment

Uh oh!

typeless Nov 22, 2016

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 3.03% (diff: 100%)

Uh oh!

strk commented Nov 22, 2016

Uh oh!

lunny commented Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typeless commented Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lunny commented Nov 22, 2016

Uh oh!

thibaultmeyer commented Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tboerger commented Nov 22, 2016

Uh oh!

lunny commented Nov 22, 2016

Uh oh!

thibaultmeyer commented Nov 22, 2016

Uh oh!

lunny commented Nov 22, 2016

Uh oh!

strk commented Nov 22, 2016

Uh oh!

lunny commented Nov 22, 2016

Uh oh!

typeless commented Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tboerger commented Nov 23, 2016

Uh oh!

typeless commented Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

strk commented Nov 23, 2016

Uh oh!

typeless commented Nov 23, 2016

Uh oh!

lunny commented Nov 23, 2016

Uh oh!

strk commented Nov 23, 2016 via email

Uh oh!

typeless commented Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typeless commented Nov 23, 2016

Uh oh!

bkcsoft commented Nov 23, 2016

Uh oh!

bkcsoft commented Nov 23, 2016

Uh oh!

typeless commented Nov 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

strk commented Nov 24, 2016 via email

Uh oh!

tboerger commented Nov 24, 2016

Uh oh!

strk commented Dec 24, 2016

Uh oh!

lunny commented Dec 24, 2016

typeless commented Nov 22, 2016 •

edited

Loading

codecov-io commented Nov 22, 2016 •

edited

Loading

lunny commented Nov 22, 2016 •

edited

Loading

typeless commented Nov 22, 2016 •

edited

Loading

thibaultmeyer commented Nov 22, 2016 •

edited

Loading

typeless commented Nov 23, 2016 •

edited

Loading

typeless commented Nov 23, 2016 •

edited

Loading

typeless commented Nov 23, 2016 •

edited

Loading

typeless commented Nov 24, 2016 •

edited

Loading