Skip to content

Commit 33b68a9

Browse files
jmillanjmillan
authored
Issue 187 (#196)
Avoid deadlock when peerconnection methods are called from the signaling thread.
1 parent e2ef364 commit 33b68a9

3 files changed

Lines changed: 119 additions & 10 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,10 @@
55
* Sync ortc with mediasoup-client ([#198](https://github.com/versatica/libmediasoupclient/pull/198)).
66
* Adapt SCTP/DataChannels API to latest changes in mediasoup ((200)[https://github.com/versatica/libmediasoupclient/pull/200]).
77

8+
### 3.5.1
9+
10+
* Fix deadlock on `PeerConnection` calls ([187](https://github.com/versatica/libmediasoupclient/issues/187)).
11+
812
### 3.5.0
913

1014
* Update to libwebrtc M140/7339 ([#173](https://github.com/versatica/libmediasoupclient/pull/188)). Credits to @revidee.

‎src/PeerConnection.cpp‎

Lines changed: 35 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,32 @@
2525

2626
using json = nlohmann::json;
2727

28+
/**
29+
* Waits for a future while pumping the current WebRTC thread's message queue
30+
* if we are running on one.
31+
*
32+
* Without this, calling PeerConnection methods from the signaling thread
33+
* deadlocks: future.get() blocks the thread, preventing it from processing
34+
* the observer callback task that would resolve the future.
35+
*/
36+
template<typename T>
37+
static T waitForFuture(std::future<T>& future)
38+
{
39+
auto* thread = webrtc::Thread::Current();
40+
41+
if (thread)
42+
{
43+
// We are on a WebRTC thread. Pump its message queue so observer
44+
// callbacks (posted as tasks) can fire while we wait.
45+
while (future.wait_for(std::chrono::milliseconds(0)) != std::future_status::ready)
46+
{
47+
thread->ProcessMessages(10);
48+
}
49+
}
50+
51+
return future.get();
52+
}
53+
2854
namespace mediasoupclient
2955
{
3056
/* Static. */
@@ -179,7 +205,7 @@ namespace mediasoupclient
179205

180206
this->pc->CreateOffer(sessionDescriptionObserver, options);
181207

182-
return future.get();
208+
return waitForFuture(future);
183209
}
184210

185211
std::string PeerConnection::CreateAnswer(
@@ -194,7 +220,7 @@ namespace mediasoupclient
194220

195221
this->pc->CreateAnswer(sessionDescriptionObserver, options);
196222

197-
return future.get();
223+
return waitForFuture(future);
198224
}
199225

200226
void PeerConnection::SetLocalDescription(webrtc::SdpType type, const std::string& sdp)
@@ -216,13 +242,13 @@ namespace mediasoupclient
216242
error.description.c_str());
217243

218244
observer->Reject(error.description);
219-
future.get();
245+
waitForFuture(future);
220246

221247
return;
222248
}
223249

224250
this->pc->SetLocalDescription(std::move(sessionDescription), observer);
225-
future.get();
251+
waitForFuture(future);
226252
}
227253

228254
void PeerConnection::SetRemoteDescription(webrtc::SdpType type, const std::string& sdp)
@@ -244,13 +270,12 @@ namespace mediasoupclient
244270
error.description.c_str());
245271

246272
observer->Reject(error.description);
247-
future.get();
248-
273+
waitForFuture(future);
249274
return;
250275
}
251276

252277
this->pc->SetRemoteDescription(std::move(sessionDescription), observer);
253-
future.get();
278+
waitForFuture(future);
254279
}
255280

256281
std::string PeerConnection::GetLocalDescription()
@@ -356,7 +381,7 @@ namespace mediasoupclient
356381

357382
this->pc->GetStats(callback.get());
358383

359-
return future.get();
384+
return waitForFuture(future);
360385
}
361386

362387
json PeerConnection::GetStats(webrtc::scoped_refptr<webrtc::RtpSenderInterface> selector)
@@ -370,7 +395,7 @@ namespace mediasoupclient
370395

371396
this->pc->GetStats(std::move(selector), callback);
372397

373-
return future.get();
398+
return waitForFuture(future);
374399
}
375400

376401
json PeerConnection::GetStats(webrtc::scoped_refptr<webrtc::RtpReceiverInterface> selector)
@@ -384,7 +409,7 @@ namespace mediasoupclient
384409

385410
this->pc->GetStats(std::move(selector), callback);
386411

387-
return future.get();
412+
return waitForFuture(future);
388413
}
389414

390415
webrtc::scoped_refptr<webrtc::DataChannelInterface> PeerConnection::CreateDataChannel(

‎test/src/Device.test.cpp‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,22 @@
11
#include "Device.hpp"
22
#include "FakeTransportListener.hpp"
33
#include "MediaSoupClientErrors.hpp"
4+
#include "api/audio_codecs/builtin_audio_decoder_factory.h"
5+
#include "api/audio_codecs/builtin_audio_encoder_factory.h"
6+
#include "api/create_peerconnection_factory.h"
7+
#include "api/video_codecs/video_decoder_factory_template.h"
8+
#include "api/video_codecs/video_decoder_factory_template_dav1d_adapter.h"
9+
#include "api/video_codecs/video_decoder_factory_template_libvpx_vp8_adapter.h"
10+
#include "api/video_codecs/video_decoder_factory_template_libvpx_vp9_adapter.h"
11+
#include "api/video_codecs/video_decoder_factory_template_open_h264_adapter.h"
12+
#include "api/video_codecs/video_encoder_factory_template.h"
13+
#include "api/video_codecs/video_encoder_factory_template_libaom_av1_adapter.h"
14+
#include "api/video_codecs/video_encoder_factory_template_libvpx_vp8_adapter.h"
15+
#include "api/video_codecs/video_encoder_factory_template_libvpx_vp9_adapter.h"
16+
#include "api/video_codecs/video_encoder_factory_template_open_h264_adapter.h"
417
#include "fakeParameters.hpp"
518
#include "ortc.hpp"
19+
#include "rtc_base/thread.h"
620
#include <catch.hpp>
721

822
TEST_CASE("Device", "[Device]")
@@ -105,3 +119,69 @@ TEST_CASE("Device", "[Device]")
105119
TransportRemoteParameters["dtlsParameters"]));
106120
}
107121
}
122+
123+
/**
124+
* Regression test: device->Load() must not deadlock when called from the
125+
* signaling thread of a custom PeerConnectionFactory.
126+
*
127+
* Root cause: PeerConnection methods used future.get() which blocked the
128+
* calling thread. WebRTC observer callbacks are delivered on the signaling
129+
* thread, so calling from that thread deadlocked.
130+
*
131+
* Fix: waitForSignalingCallback() pumps the current WebRTC thread's message
132+
* queue while waiting, allowing observer callbacks to fire.
133+
*
134+
* https://github.com/versatica/libmediasoupclient/issues/187
135+
*/
136+
TEST_CASE("Device::Load from signaling thread does not deadlock", "[Device]")
137+
{
138+
auto networkThread = webrtc::Thread::CreateWithSocketServer();
139+
auto workerThread = webrtc::Thread::Create();
140+
auto signalingThread = webrtc::Thread::Create();
141+
networkThread->SetName("deadlock_test_network", nullptr);
142+
workerThread->SetName("deadlock_test_worker", nullptr);
143+
signalingThread->SetName("deadlock_test_signaling", nullptr);
144+
networkThread->Start();
145+
workerThread->Start();
146+
signalingThread->Start();
147+
148+
auto factory = webrtc::CreatePeerConnectionFactory(
149+
networkThread.get(),
150+
workerThread.get(),
151+
signalingThread.get(),
152+
nullptr,
153+
webrtc::CreateBuiltinAudioEncoderFactory(),
154+
webrtc::CreateBuiltinAudioDecoderFactory(),
155+
std::make_unique<webrtc::VideoEncoderFactoryTemplate<
156+
webrtc::LibvpxVp8EncoderTemplateAdapter,
157+
webrtc::LibvpxVp9EncoderTemplateAdapter,
158+
webrtc::OpenH264EncoderTemplateAdapter,
159+
webrtc::LibaomAv1EncoderTemplateAdapter>>(),
160+
std::make_unique<webrtc::VideoDecoderFactoryTemplate<
161+
webrtc::LibvpxVp8DecoderTemplateAdapter,
162+
webrtc::LibvpxVp9DecoderTemplateAdapter,
163+
webrtc::OpenH264DecoderTemplateAdapter,
164+
webrtc::Dav1dDecoderTemplateAdapter>>(),
165+
nullptr,
166+
nullptr,
167+
nullptr,
168+
nullptr);
169+
170+
REQUIRE(factory != nullptr);
171+
172+
mediasoupclient::PeerConnection::Options pcOptions;
173+
pcOptions.factory = factory.get();
174+
175+
// Call device->Load() FROM the signaling thread - the exact scenario
176+
// from the bug report.
177+
bool loaded{ false };
178+
signalingThread->BlockingCall(
179+
[&]()
180+
{
181+
mediasoupclient::Device device;
182+
REQUIRE_NOTHROW(device.Load(generateRouterRtpCapabilities(), &pcOptions));
183+
loaded = device.IsLoaded();
184+
});
185+
186+
REQUIRE(loaded);
187+
}

0 commit comments

Comments
 (0)