diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a99d869..bdfb24b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,10 @@ # Changelog +### 3.5.1 + +* Fix deadlock on `PeerConnection` calls ([187](https://github.com/versatica/libmediasoupclient/issues/187)). + ### 3.5.0 * Update to libwebrtc M140/7339 ([#173](https://github.com/versatica/libmediasoupclient/pull/188)). Credits to @revidee. diff --git a/src/PeerConnection.cpp b/src/PeerConnection.cpp index 5d19258e..7a0eae7e 100644 --- a/src/PeerConnection.cpp +++ b/src/PeerConnection.cpp @@ -24,6 +24,32 @@ using json = nlohmann::json; +/** + * Waits for a future while pumping the current WebRTC thread's message queue + * if we are running on one. + * + * Without this, calling PeerConnection methods from the signaling thread + * deadlocks: future.get() blocks the thread, preventing it from processing + * the observer callback task that would resolve the future. + */ +template +static T waitForFuture(std::future& future) +{ + auto* thread = webrtc::Thread::Current(); + + if (thread) + { + // We are on a WebRTC thread. Pump its message queue so observer + // callbacks (posted as tasks) can fire while we wait. + while (future.wait_for(std::chrono::milliseconds(0)) != std::future_status::ready) + { + thread->ProcessMessages(10); + } + } + + return future.get(); +} + namespace mediasoupclient { /* Static. */ @@ -178,7 +204,7 @@ namespace mediasoupclient this->pc->CreateOffer(sessionDescriptionObserver, options); - return future.get(); + return waitForFuture(future); } std::string PeerConnection::CreateAnswer( @@ -193,7 +219,7 @@ namespace mediasoupclient this->pc->CreateAnswer(sessionDescriptionObserver, options); - return future.get(); + return waitForFuture(future); } void PeerConnection::SetLocalDescription(webrtc::SdpType type, const std::string& sdp) @@ -215,13 +241,13 @@ namespace mediasoupclient error.description.c_str()); observer->Reject(error.description); - future.get(); + waitForFuture(future); return; } this->pc->SetLocalDescription(std::move(sessionDescription), observer); - future.get(); + waitForFuture(future); } void PeerConnection::SetRemoteDescription(webrtc::SdpType type, const std::string& sdp) @@ -243,13 +269,12 @@ namespace mediasoupclient error.description.c_str()); observer->Reject(error.description); - future.get(); - + waitForFuture(future); return; } this->pc->SetRemoteDescription(std::move(sessionDescription), observer); - future.get(); + waitForFuture(future); } std::string PeerConnection::GetLocalDescription() @@ -355,7 +380,7 @@ namespace mediasoupclient this->pc->GetStats(callback.get()); - return future.get(); + return waitForFuture(future); } json PeerConnection::GetStats(webrtc::scoped_refptr selector) @@ -369,7 +394,7 @@ namespace mediasoupclient this->pc->GetStats(std::move(selector), callback); - return future.get(); + return waitForFuture(future); } json PeerConnection::GetStats(webrtc::scoped_refptr selector) @@ -383,7 +408,7 @@ namespace mediasoupclient this->pc->GetStats(std::move(selector), callback); - return future.get(); + return waitForFuture(future); } webrtc::scoped_refptr PeerConnection::CreateDataChannel( diff --git a/test/src/Device.test.cpp b/test/src/Device.test.cpp index 49f250ad..b259a241 100644 --- a/test/src/Device.test.cpp +++ b/test/src/Device.test.cpp @@ -1,8 +1,22 @@ #include "Device.hpp" #include "FakeTransportListener.hpp" #include "MediaSoupClientErrors.hpp" +#include "api/audio_codecs/builtin_audio_decoder_factory.h" +#include "api/audio_codecs/builtin_audio_encoder_factory.h" +#include "api/create_peerconnection_factory.h" +#include "api/video_codecs/video_decoder_factory_template.h" +#include "api/video_codecs/video_decoder_factory_template_dav1d_adapter.h" +#include "api/video_codecs/video_decoder_factory_template_libvpx_vp8_adapter.h" +#include "api/video_codecs/video_decoder_factory_template_libvpx_vp9_adapter.h" +#include "api/video_codecs/video_decoder_factory_template_open_h264_adapter.h" +#include "api/video_codecs/video_encoder_factory_template.h" +#include "api/video_codecs/video_encoder_factory_template_libaom_av1_adapter.h" +#include "api/video_codecs/video_encoder_factory_template_libvpx_vp8_adapter.h" +#include "api/video_codecs/video_encoder_factory_template_libvpx_vp9_adapter.h" +#include "api/video_codecs/video_encoder_factory_template_open_h264_adapter.h" #include "fakeParameters.hpp" #include "ortc.hpp" +#include "rtc_base/thread.h" #include TEST_CASE("Device", "[Device]") @@ -105,3 +119,69 @@ TEST_CASE("Device", "[Device]") TransportRemoteParameters["dtlsParameters"])); } } + +/** + * Regression test: device->Load() must not deadlock when called from the + * signaling thread of a custom PeerConnectionFactory. + * + * Root cause: PeerConnection methods used future.get() which blocked the + * calling thread. WebRTC observer callbacks are delivered on the signaling + * thread, so calling from that thread deadlocked. + * + * Fix: waitForSignalingCallback() pumps the current WebRTC thread's message + * queue while waiting, allowing observer callbacks to fire. + * + * https://github.com/versatica/libmediasoupclient/issues/187 + */ +TEST_CASE("Device::Load from signaling thread does not deadlock", "[Device]") +{ + auto networkThread = webrtc::Thread::CreateWithSocketServer(); + auto workerThread = webrtc::Thread::Create(); + auto signalingThread = webrtc::Thread::Create(); + networkThread->SetName("deadlock_test_network", nullptr); + workerThread->SetName("deadlock_test_worker", nullptr); + signalingThread->SetName("deadlock_test_signaling", nullptr); + networkThread->Start(); + workerThread->Start(); + signalingThread->Start(); + + auto factory = webrtc::CreatePeerConnectionFactory( + networkThread.get(), + workerThread.get(), + signalingThread.get(), + nullptr, + webrtc::CreateBuiltinAudioEncoderFactory(), + webrtc::CreateBuiltinAudioDecoderFactory(), + std::make_unique>(), + std::make_unique>(), + nullptr, + nullptr, + nullptr, + nullptr); + + REQUIRE(factory != nullptr); + + mediasoupclient::PeerConnection::Options pcOptions; + pcOptions.factory = factory.get(); + + // Call device->Load() FROM the signaling thread - the exact scenario + // from the bug report. + bool loaded{ false }; + signalingThread->BlockingCall( + [&]() + { + mediasoupclient::Device device; + REQUIRE_NOTHROW(device.Load(generateRouterRtpCapabilities(), &pcOptions)); + loaded = device.IsLoaded(); + }); + + REQUIRE(loaded); +}