Skip to content
This repository has been archived by the owner on Mar 26, 2020. It is now read-only.

Fix #1321: remove session references #1371

Merged
merged 1 commit into from Dec 7, 2018
Merged

Conversation

bserdar
Copy link
Contributor

@bserdar bserdar commented Dec 6, 2018

acceptLoop keeps a list of all sessions, and each session keeps a reference to the memory block allocated as a read buffer. This reference is there even if the session closes, so as long as acceptLoop runs, memory keeps growing. This should fix it by doing two things: 1) When session closes, remove the reference to the buffer. But that will still continue to leak sessions, because nothing removes elements from the session list. So, 2) remove the logic that keep track of sessions, instead, run two goroutines for each session, one to keep the session and run the server, and the other to terminate the first one when needed.

I don't have the infra to verify this, so let me know how it goes.

…ession keeps a reference to the memory block allocated as a read buffer. This reference is there even if the session closes, so as long as acceptLoop runs, memory keeps growing. This should fix it by doing two things: 1) When session closes, remove the reference to the buffer. But that will still continue to leak sessions, because nothing removes elements from the session list. So, 2) remove the logic that keep track of sessions, instead, run two goroutines for each session, one to keep the session and run the server, and the other to terminate the first one when needed.
@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@rishubhjain
Copy link
Contributor

rishubhjain commented Dec 6, 2018

@bserdar Does this PR help in reducing memory leak?

@bserdar
Copy link
Contributor Author

bserdar commented Dec 6, 2018

There are two leaks, the larger one is related to the read buffer in ServerCodec. It is not freed, because a reference to the buffer is kept in the servercodec, acceptLoop keeps a list of all servercodecs even if they're closed. so, the 1-liner in servercodec.go should remove the read buffer reference when the session is closed, and the two goroutines in the acceptLoop should eliminate the need to keep all the sessions.

@rishubhjain
Copy link
Contributor

Fixes:#1321 @harigowtham Please check

@atinmu
Copy link
Contributor

atinmu commented Dec 6, 2018

@prashanthpai if possible, please review!

Copy link
Member

@harigowtham harigowtham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes make sense to remove the unnecessary allocations. I'll try it out and post the finding.

I'm relatively new to this portion of the code and I have commented my question as a part of this review.

@@ -132,17 +132,10 @@ func (s *SunRPC) acceptLoop(stopCh chan struct{}, l net.Listener, wg *sync.WaitG
"transport": ltype})
logger.WithField("address", l.Addr().String()).Info("started server")

sessions := make([]rpc.ServerCodec, 50)
for {
select {
case <-stopCh:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't this implementation make this code useless?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The select on stopCh is still required to return from the acceptLoop. There is no other return from this function. If stopCh is closed, acceptLoop will terminate, along with all the sessions that are still active.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got the return in the func confused with the return for this infinte loop.

Copy link
Member

@harigowtham harigowtham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the initial test run for the patch, I don't see a leak. Will test it further. We will see if others have any comments.

@atinmu
Copy link
Contributor

atinmu commented Dec 7, 2018

Functionality wise changes look fine.

@aravindavk @Madhu-1 please review?

@aravindavk
Copy link
Member

add to whitelist

@aravindavk
Copy link
Member

retest this please

Copy link
Member

@aravindavk aravindavk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@atinmu atinmu merged commit b73370c into gluster:master Dec 7, 2018
@atinmu
Copy link
Contributor

atinmu commented Dec 7, 2018

Fixes gluster/gcs#76 , gluster/gcs#71

@aravindavk
Copy link
Member

Thanks @bserdar for the PR. Two go routine approach is nice and helpful.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants