search

jitsi / libjitsi

Advanced Java media library for secure real-time audio/video communication.
Apache License 2.0
627 stars 280 forks source link

fix: Initialize the zrtp timeout thred only when needed. #544

Closed damencho closed 3 years ago

damencho commented 3 years ago

In jigasi we saw some TimeoutProvider threads leaking ... where zrtp is not used. Seems there is a scenario in which the ZRTPTransformEngine is initialized but never stopped/cleaned. And zrtp is not used there. Rarely happening, but still happening 20 times out of 2k.

ibauersachs commented 3 years ago

It'd probably be even better to avoid creating the ZRTP engine and SrtpControl at all. Not sure why that's happening when ZRTP is disabled. Maybe this needs to be changed to the NullControl (which doesn't do encryption):

https://github.com/jitsi/libjitsi/blob/d1f44f8eed24e3e45270e459e26f23d3e75d0ca1/src/org/jitsi/impl/neomedia/MediaStreamImpl.java#L413-L417

I'm not sure anymore what the comment just above refers to, probably this:

https://github.com/jitsi/jitsi/blob/eb00506cd2fd9f06325ee3dc3cd1826ccf919f67/src/net/java/sip/communicator/service/protocol/media/MediaHandler.java#L927-L935

damencho commented 3 years ago

Yeah, I was thinking the same, why we even create it ... :)

damencho commented 3 years ago

@gpolitis @ibauersachs So in my tests I always had DEFAULT_ENCRYPTION=false and with this change the NullControl is used and if I comment DEFAULT_ENCRYPTION setting, leaving default zrtp is initialized as before. And about the comment, I saw nothing that needs to be adjusted 🤷‍♂️

damencho commented 3 years ago

Thanks @ibauersachs