GRVYDEV / Lightspeed-webrtc

A RTP -> WebRTC broadcast server for Project Lightspeed.
MIT License
75 stars 31 forks source link

Simpler and more efficient cleanConnections #20

Closed connor4312 closed 3 years ago

connor4312 commented 3 years ago

I was glancing through the code and was interested by this function.

I think it can be simplified in this way -- even if the connection is blocking on a Remove/AddTrack at the instant, the peer connection still signaled it closed so I think it'd be appropriate to boot it out of the list instantly? Which as far I as I see is only used for telemetry at the moment. (A prometheus counter might actually be what you want 🙂 )

Also, I noticed you were using order-preserving delete, which required reallocating, or at least copying, the entire list if you removed the first element. But you don't need to preserve order, so you can do a simple cheap swap instead.

GRVYDEV commented 3 years ago

I was glancing through the code and was interested by this function.

I think it can be simplified in this way -- even if the connection is blocking on a Remove/AddTrack at the instant, the peer connection still signaled it closed so I think it'd be appropriate to boot it out of the list instantly? Which as far I as I see is only used for telemetry at the moment. (A prometheus counter might actually be what you want slightly_smiling_face )

Also, I noticed you were using order-preserving delete, which required reallocating, or at least copying, the entire list if you removed the first element. But you don't need to preserve order, so you can do a simple cheap swap instead.

Hi! Thank you for this any your explanation. Essentially the count of the connection list will be pushed to everyone connected to the websocket in order to be used as the "viewer" count. I agree that the list cleaning function could be simplified and your code looks great! Thank you!

connor4312 commented 3 years ago

If you just need a counter, I suggest using sync/atomic (or a normal uint and a sync.Cond if you need signalling as well).