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

memory overflow #1622

Closed
fayfive opened this issue Jan 2, 2021 · 6 comments · Fixed by #1623
Closed

memory overflow #1622

fayfive opened this issue Jan 2, 2021 · 6 comments · Fixed by #1623

Comments

@fayfive
Copy link

fayfive commented Jan 2, 2021

I wrote a program referring to the V3 broadcast example. Read local h264 to WriteSample(). The function is OK, but the memory can not be released. When a old watcher web page close, the memory can not be reduced. When a new watcher page open, the memory is increased. I use OnConnectionStateChange to listen PeerConnectionStateDisconnected. When PeerConnectionStateDisconnected, I RemoveTrack and close peerconnection.

pc.OnConnectionStateChange(func(state webrtc.PeerConnectionState) {
                if state == webrtc.PeerConnectionStateDisconnected {
			chanClose <- true
		}
	})
<-chanClose
if senders := pc.GetSenders(); len(senders) != 0 {
			for _, s := range senders {
				if err := pc.RemoveTrack(s); err != nil {
					log.Println(err)
				}
			}
		}
		if pc != nil {
			err := pc.Close()
			if err != nil {
				log.Println(err)
			}
		}
@Sean-Der
Copy link
Member

Sean-Der commented Jan 2, 2021

Hey @fayfive

Sorry you are having issues! You don't need to call RemoveTrack, closing the PeerConnection will free all resources associated with the PeerConnection.

I am not able to reproduce this right now. We have some basic tests to assert things don't leak, but we could have missed something.

Would you mind doing a heap profile? If you are able to share the entire project that would even be better! I am happy to run it and debug also.

thanks

@fayfive
Copy link
Author

fayfive commented Jan 3, 2021

@Sean-Der
Your sample has the same phenomenon. It also listen disconnected then peerConnection.Close(), but the memory can not be released. My RTSP just has H264 and no voice.

Here is the link
https://github.com/pion/rtsp-bench

thanks

@Sean-Der
Copy link
Member

Sean-Der commented Jan 3, 2021

I am able to reproduce this. The NACK Responder Interceptor has a leak.

The issue is that sync.Map is GC'ed this seems to be related to golang/go#40999 I am moving to a map + mutex

Sean-Der added a commit to pion/interceptor that referenced this issue Jan 3, 2021
sync.Map is causing confusion around garbage collection so just removing
the complexity all together.

Relates to pion/webrtc#1622
Sean-Der added a commit that referenced this issue Jan 3, 2021
Drops sync.Map usage and fixes leaks

Resolves #1622
Sean-Der added a commit that referenced this issue Jan 3, 2021
Drops sync.Map usage and fixes leaks

Resolves #1622
@Sean-Der
Copy link
Member

Sean-Der commented Jan 3, 2021

@fayfive This is fixed with v3.0.2 the fix is pion/interceptor@edb026a

sync.Map would only do GC in certain cases. I was able to confirm that it would leak sometimes.

Another thing I saw is that if a HTTP Request hung (SIGQUIT the client) that that PeerConnection created in the handler would stick even if Close had been called. Make sure that you gracefully kill all HTTP clients or wait until the HTTP request fully times out.

@Sean-Der
Copy link
Member

Sean-Der commented Jan 3, 2021

To confirm I did the following

  • I loaded the page in 3 tabs in Chromium and ran go tool pprof -png -nodecount 0 'http://localhost:8080/debug/pprof/heap' to confirm things were allocated

  • I ran the same command and got the attached heap profile

profile001

@fayfive
Copy link
Author

fayfive commented Jan 4, 2021

@Sean-Der
The bug is fixed. Thanks!
I am using sync.Map(go version 1.15.5) in my project and the project is OK. I saw the sysnc.Map issue, it seem to be fixed in Augest.

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

Successfully merging a pull request may close this issue.

3 participants
@Sean-Der @fayfive and others