fix: whep gathering failure leaks peer connections

[RouquinBlanc]

Tentative fix for #3118

It still leaks, but less; Here is what remains leaking:

goroutine profile: total 128
50 @ 0x100843218 0x10083cae8 0x100876730 0x1008b8538 0x1008b9880 0x1008b9871 0x100a20cb8 0x100a32234 0x100b5ddd0 0x100ac7488 0x100ac75f0 0x100b5ed58 0x10087d084
#	0x10087672f	internal/poll.runtime_pollWait+0x9f		/opt/homebrew/opt/go/libexec/src/runtime/netpoll.go:345
#	0x1008b8537	internal/poll.(*pollDesc).wait+0x27		/opt/homebrew/opt/go/libexec/src/internal/poll/fd_poll_runtime.go:84
#	0x1008b987f	internal/poll.(*pollDesc).waitRead+0x1ff	/opt/homebrew/opt/go/libexec/src/internal/poll/fd_poll_runtime.go:89
#	0x1008b9870	internal/poll.(*FD).Read+0x1f0			/opt/homebrew/opt/go/libexec/src/internal/poll/fd_unix.go:164
#	0x100a20cb7	net.(*netFD).Read+0x27				/opt/homebrew/opt/go/libexec/src/net/fd_posix.go:55
#	0x100a32233	net.(*conn).Read+0x33				/opt/homebrew/opt/go/libexec/src/net/net.go:179
#	0x100b5ddcf	net/http.(*persistConn).Read+0x4f		/opt/homebrew/opt/go/libexec/src/net/http/transport.go:1976
#	0x100ac7487	bufio.(*Reader).fill+0xf7			/opt/homebrew/opt/go/libexec/src/bufio/bufio.go:110
#	0x100ac75ef	bufio.(*Reader).Peek+0x5f			/opt/homebrew/opt/go/libexec/src/bufio/bufio.go:148
#	0x100b5ed57	net/http.(*persistConn).readLoop+0x157		/opt/homebrew/opt/go/libexec/src/net/http/transport.go:2140

50 @ 0x100843218 0x1008566b8 0x100b60600 0x10087d084
#	0x100b605ff	net/http.(*persistConn).writeLoop+0x9f	/opt/homebrew/opt/go/libexec/src/net/http/transport.go:2443

[codecov]

Codecov Report

Attention: Patch coverage is 42.30769% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 55.57%. Comparing base (f8d2343) to head (fa24957).

Files	Patch %	Lines
internal/protocols/webrtc/whip_client.go	0.00%	15 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3124      +/-   ##
==========================================
- Coverage   55.57%   55.57%   -0.01%     
==========================================
  Files         150      150              
  Lines       16822    16839      +17     
==========================================
+ Hits         9349     9358       +9     
- Misses       6735     6744       +9     
+ Partials      738      737       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[RouquinBlanc]

Second part seems to do the trick!

Still leaking... Same thing, it's just slow to build up

[RouquinBlanc]

Seems fine now (running 20 minutes and no increase).

It looks like the HTTPClient keeps idle open connections for future requests and does not clean them up unless CloseIdleConnections() is called. They would eventually die out, but that depends on GC and/or remote side closing, which is not ideal.

🤞

[aler9]

Thanks for this patch, i did the following changes:

• i moved http.Client.CloseIdleConnections() outside WHIPClient, since the HTTP Client is not meant as a managed component of WHIPClient but as an external component that may be shared with other entities, therefore WHIPClient should not perform actions that may interfere with other entities
• i edited WHIPClient.Publish() and WHIPClient.Read() in order to automatically call WHIPClient.Close(), therefore calling it manually should not be necessary anymore

I agree with you on the fact that http.Client.CloseIdleConnections() should be called whenever a http.Client is not in use anymore, i'll edit the server in order to add this in all other places where http.Client is in use.

This has been merged, thanks!

[github-actions]

This issue is mentioned in release v1.7.0 🚀
Check out the entire changelog by clicking here