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

Large repo push http : consumption of ram #636

Closed
Timetopretend opened this Issue Nov 19, 2014 · 16 comments

Comments

4 participants
@Timetopretend

Timetopretend commented Nov 19, 2014

I tried to push a large repository via http. Everything goes well until the transfer is finished eating ram becomes important (Gogs). It's a shame Gogs is so light yet in other cases. To see if this is not a technical limitation caused by the protocol, but I think it is possible to optimize.

@Unknwon Unknwon added the kind/bug label Nov 19, 2014

@Unknwon

This comment has been minimized.

Member

Unknwon commented Nov 19, 2014

Thanks your feedback!

There are many reasons and we're actually tracing on it at some level:

  1. Go GC doesn't return RAM back to system in time(so we force to GC in the latest master code)
  2. Bad implementation on HTTP receive(doesn't use memory wisely)

I'll keep this issue open for now.

@bkcsoft

This comment has been minimized.

Contributor

bkcsoft commented May 26, 2016

https://github.com/gogits/gogs/blob/master/routers/repo/http.go#L362 input, err = ioutil.ReadAll(reqBody) is the line that makes Gogs use a lot of RAM when pushing/pulling over HTTP. Not sure why that block is required either to be honest @Unknwon ?

@Unknwon

This comment has been minimized.

Member

Unknwon commented May 30, 2016

@bkcsoft good point! Maybe you could try to use a continuous reader see if it works?

@bkcsoft

This comment has been minimized.

Contributor

bkcsoft commented May 30, 2016

@bkcsoft

This comment has been minimized.

Contributor

bkcsoft commented May 30, 2016

This will be a much larger task than I anticipated... Involving goroutines and channels 😢

@Timetopretend

This comment has been minimized.

Timetopretend commented May 30, 2016

Excited to see movement here :)

@bkcsoft

This comment has been minimized.

Contributor

bkcsoft commented May 31, 2016

@Unknwon is there a reason for not using the post-recieve-hook instead of the callback that requires that line? seems like it would be way easier to implement it correctly that way 😃

@Unknwon

This comment has been minimized.

Member

Unknwon commented Jun 1, 2016

@bkcsoft no worries, I'll take a deep look. Not sure why does it matter with post-receive hook?

@Unknwon

This comment has been minimized.

Member

Unknwon commented Jun 1, 2016

Clean up the code for http.go a bit: c041273

Still working on the input, err = ioutil.ReadAll(reqBody) thing.

@bkcsoft

This comment has been minimized.

Contributor

bkcsoft commented Jun 2, 2016

The issue is the callback is reading refname, oldsum and newsum from the http-request. This is why it needs to store it (unless you do magic), unless you use the post-recieve-hook which would give us this automagically :)

Also, I'm still working on this 😆

EDIT: I'm actually almost done, both clone and push are fixed, just that it isn't closing the http-response which messes with the git-client 😒

@Unknwon

This comment has been minimized.

Member

Unknwon commented Jun 2, 2016

@bkcsoft oops, hope does not break your code too much.... 😓

@bkcsoft

This comment has been minimized.

Contributor

bkcsoft commented Jun 3, 2016

@strk

This comment has been minimized.

Contributor

strk commented Sep 8, 2016

I took a look and wondering why ReadAll is needed at all.
The code is finally creating a Reader out of the buffer, why not directly using reqBody as the Reader ?
It is originally an io.ReadCloser...

@Unknwon

This comment has been minimized.

Member

Unknwon commented Dec 24, 2016

This is fixed by #3748.

@Unknwon

This comment has been minimized.

Member

Unknwon commented Feb 14, 2017

Just implemented Git hook delegation, so this problem can be ultimately solved by post-receive hook. Reopen as a reminder for myself.

@Unknwon Unknwon reopened this Feb 14, 2017

@Unknwon Unknwon modified the milestones: 0.11.0, 0.10.0 Feb 16, 2017

Unknwon added a commit that referenced this issue Feb 16, 2017

refactoring: SSH and HTTP push procees is now unified
We used to handle SSH and HTTP push separately which produces
duplicated code, but now with post-receive hook, the process
is unified to one single place and much cleaner.
Thus, UpdateTask struct is removed.

Narrow down the range of Git HTTP routes to reduce condufsing
HTTP Basic Authentication window popup on browser.

By detecting <old-commit, new-commit, ref-name> inside post-receive
hook, Git HTTP doesn't need to read the whole content body anymore,
which completely solve the RAM problem reported in #636.
@Unknwon

This comment has been minimized.

Member

Unknwon commented Feb 16, 2017

This problem should be completely solved by d521e71.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment