search

versatica / libmediasoupclient

mediasoup client side C++ library
https://mediasoup.org
ISC License
286 stars 177 forks source link

Set PeerConnection threads to nullptr when no PeerConnection::Options is passed to constructor #122

Closed pablito25sp closed 3 years ago

pablito25sp commented 3 years ago

When creating a new PeerConnection without PeerConnection::Options, set threads to nullptr so default destructor can take care of it on PeerConnection destruction.

pablito25sp commented 3 years ago

Unrelated to this PR, but related with memory leaks, check this out: https://github.com/nlohmann/json/issues/1971 https://github.com/nlohmann/json/commit/e4d8dc02e87614854f82f2d960c1e38a10511519

I was having leaks related to that, so I patched include/nlohmann/json.hpp and they're gone. Let me know if you want me to put that change in this PR as well. Best!

jmillan commented 3 years ago

@pablito25sp,

Please, don't mix different tasks in the same PR.

Please keep in this PR just the changes for PeerConnection.

Thanks!

pablito25sp commented 3 years ago

@jmillan sorry about mixing. Reverted las commit. Best regards

jmillan commented 3 years ago

@pablito25sp,

This is not right, sorry.

Thread members in 'PeerConnection' are hold in `std::unique_ptr', which means they are destructed when 'PeerConnection' gets out of the scope.

Besides, this PR set's them to nullptr on constructor, so either way it does really nothing.

If you have some info of the initial issue (memory leak?) please comment that in a separate issue.

pablito25sp commented 3 years ago

Ok, not a C++ expert here. I assigned threads to nullptr to avoid dynamically assignment after reading this:

If we do not write our own destructor in class, compiler creates a default destructor for us. **The default destructor works fine unless we have dynamically allocated memory or pointer in class.** When a class contains a pointer to memory allocated in class, we should write a destructor to release memory before the class instance is destroyed. This must be done to avoid memory leak.

Source: https://www.geeksforgeeks.org/destructors-c/

Then I tested the solution within my implementation and it worked. If you're right, then I don't know why this PR works.

pablito25sp commented 3 years ago

Will debug and investigate deeper. I don't wanna waste your time. Sorry for the inconveniences.

ibc commented 3 years ago

As @jmillan said, we use std::unique_por for those pointers so it's guaranteed that those are deallocated when the class instance is freed.