Skip to content
This repository has been archived by the owner on Jun 21, 2022. It is now read-only.

Implement StreamMuxer trait for Connection #2

Merged
merged 26 commits into from Jun 17, 2022
Merged

Implement StreamMuxer trait for Connection #2

merged 26 commits into from Jun 17, 2022

Conversation

melekes
Copy link
Owner

@melekes melekes commented May 24, 2022

@melekes
Copy link
Owner Author

melekes commented May 24, 2022

@tomaka could you please take a look at this when you have the time? Thanks.

src/connection.rs Outdated Show resolved Hide resolved
src/connection.rs Outdated Show resolved Hide resolved
src/connection.rs Outdated Show resolved Hide resolved
@tomaka
Copy link
Collaborator

tomaka commented May 25, 2022

The multiple mutexes feel a bit dangerous to me, both in terms of deadlock and in terms of race conditions (what if another method is called between the moment a mutex is unlocked and the next one locked? is it ok everywhere?)
I think that a single mutex covering all the fields is much safer and probably not much slower.

@tomaka
Copy link
Collaborator

tomaka commented May 25, 2022

Another point is that I would suggest keeping the single data stream implementation on the side.
I feel like I'm getting ignored, but to me it's a good idea to also give the possibility to have this single data stream system.

@melekes
Copy link
Owner Author

melekes commented May 25, 2022

I think that a single mutex covering all the fields is much safer and probably not much slower.

This is how it was done before. Unfortunately, I had to switch to multiple mutexes (futures::Mutex and std::Mutex) because I was not able to get both async and sync code to compile with just one (I've tried both futures::Mutex and std::sync::Mutex).

If I use futures::Mutex, it works inside async future, but I had to poll it in sync code and I wasn't sure how it will play with the rest of the code (i.e. if polling future returns ready, but mutex is not ready, will the future returns ready next time it's called?).

If I use std::sync::Mutex, I have problems when locking data across await, I believe.

I can try again though.

@melekes
Copy link
Owner Author

melekes commented May 25, 2022

Another point is that I would suggest keeping the single data stream implementation on the side. I feel like I'm getting ignored, but to me it's a good idea to also give the possibility to have this single data stream system.

It's still possible to do with the new implementation. You get a Connection and call open_outbound substream. Then you use it to communicate with the other party. Or you can open additional data channels (it's up to you).

@melekes melekes marked this pull request as draft May 25, 2022 10:07
@melekes melekes self-assigned this May 26, 2022
It turned out we actually don't need it. webrtc-rs lib will increase
channel ID for us (on both ends on the connection).

Also, extract `register_data_channel_open_handler` fn to reduce the
amount of duplicated code.
src/transport.rs Outdated Show resolved Hide resolved
@melekes melekes marked this pull request as ready for review June 17, 2022 09:51
@melekes melekes merged commit 0d85a4a into main Jun 17, 2022
@melekes melekes deleted the streammuxer branch June 17, 2022 09:52
melekes added a commit to melekes/rust-libp2p that referenced this pull request Jun 20, 2022
Copied from melekes/libp2p-webrtc-direct#2.

This code does not yet compiles because of `XWebRTC` protocol. if you want to see the working
version, please visit https://github.com/melekes/libp2p-webrtc-direct.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants