Skip to content
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 sure that we send NewChannel before any other event #364

Closed
wants to merge 3 commits into from

Conversation

@ferjm
Copy link
Member

ferjm commented Jun 24, 2020

No description provided.

@ferjm ferjm requested a review from jdm Jun 24, 2020
@ferjm
Copy link
Member Author

ferjm commented Jun 24, 2020

r? @jdm

I think this fixes the issue that you mentioned here

@@ -76,6 +76,7 @@ pub struct GStreamerWebRtcController {
remote_offer_generation: u32,
_main_loop: glib::MainLoop,
data_channels: Arc<Mutex<HashMap<DataChannelId, GStreamerWebRtcDataChannel>>>,
data_channels_event_queue: Arc<Mutex<HashMap<DataChannelId, Vec<DataChannelEvent>>>>,

This comment has been minimized.

@Manishearth

Manishearth Jun 24, 2020

Member

A concurrent queue is just a sender/receiver pair

data_channels: Arc<Mutex<HashMap<DataChannelId, GStreamerWebRtcDataChannel>>>,
data_channels_event_queue: Arc<Mutex<HashMap<DataChannelId, Vec<DataChannelEvent>>>>,
Comment on lines 78 to 79

This comment has been minimized.

@jdm

jdm Jun 25, 2020

Member

I get a little bit nervous about races when there are multiple independent locks for what is essentially the same data. Is there any way to do something like:

enum DataChannelState {
    Buffered(Vec<DataChannelEvent>),
    Created(GStreamerWebRtcDataChannel),
}

And then only have a single hashmap?

This comment has been minimized.

@Manishearth

Manishearth Jun 25, 2020

Member

I think we can have a single hashmap adn then just a Sender/Receiver pair, the vec is being used as a queue.

But the enum works too

backends/gstreamer/webrtc.rs Show resolved Hide resolved
match channel {
DataChannelEventTarget::Buffered(ref mut events) => {
for event in events.drain(0..) {
thread_.internal_event(

This comment has been minimized.

@jdm

jdm Jun 26, 2020

Member

Is there any chance that we could have a buffered close event? If so, we will end up registering a channel after the close.

@jdm
Copy link
Member

jdm commented Jun 26, 2020

r=me when you're satisfied with the answer to my question.

@ferjm
Copy link
Member Author

ferjm commented Jun 29, 2020

@bors-servo r=jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jun 29, 2020

📌 Commit fbb3082 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jun 29, 2020

Testing commit fbb3082 with merge 6aa53e1...

bors-servo added a commit that referenced this pull request Jun 29, 2020
Make sure that we send NewChannel before any other event
@bors-servo
Copy link
Contributor

bors-servo commented Jun 29, 2020

☀️ Test successful - checks-travis
Approved by: jdm
Pushing 6aa53e1 to master...

bors-servo added a commit that referenced this pull request Jun 29, 2020
Do not register channel with buffered closed event

This commit was supposed to be part of #364, but it seems that Github had service issues and lost the last commit :\
@ferjm
Copy link
Member Author

ferjm commented Jul 2, 2020

This was merged already 6aa53e1

@ferjm ferjm closed this Jul 2, 2020
@ferjm ferjm deleted the ferjm:datachannels.event.queue branch Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.