-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make getLocalSdpInfo async #1441
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM mod comments
@@ -178,6 +178,7 @@ bool WebRtcConnection::createOfferSync(bool video_enabled, bool audio_enabled, b | |||
|
|||
boost::future<void> WebRtcConnection::addMediaStream(std::shared_ptr<MediaStream> media_stream) { | |||
return asyncTask([media_stream] (std::shared_ptr<WebRtcConnection> connection) { | |||
boost::mutex::scoped_lock lock(connection->update_state_mutex_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this lock here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it for consistency with addMediaStream
.
boost::mutex::scoped_lock lock(mutex); | ||
if (!future_async_) { | ||
return; | ||
} | ||
futures.push(persistent); | ||
ResultPair result_pair(persistent, result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
v8::Local<v8::Promise::Resolver> resolver = Nan::New(*persistent); | ||
resolver->Resolve(Nan::GetCurrentContext(), Nan::New("").ToLocalChecked()); | ||
ResultVariant r = obj->futures.front().second; | ||
if (boost::get<std::string>(&r) != nullptr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be possible to do it with void instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
} else if (boost::get<std::shared_ptr<erizo::SdpInfo>>(&r) != nullptr) { | ||
std::shared_ptr<erizo::SdpInfo> sdp_info = boost::get<std::shared_ptr<erizo::SdpInfo>>(r); | ||
v8::Local<v8::Object> instance = ConnectionDescription::NewInstance(); | ||
ConnectionDescription* description = ObjectWrap::Unwrap<ConnectionDescription>(instance); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to do this object persistent or something similar to avoid having issues with the garbage collector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've kept the fuctionality exactly the same as we had. I'm not 100% sure it's the best way to do this, though. We will have to keep an eye on it
Description
This PR addresses race conditions when getting and handling SdpInfo by making it run as an
asyncTask
in erizo. This involves also implementing a mechanism to return values when promises are resolved from ErizoAPI.[] It needs and includes Unit Tests
Changes in Client or Server public APIs
[] It includes documentation for these changes in
/doc
.