Comments (16)
@ggarber I hate you. I'll refactor the PR to follow your proposal.
from mediasoup.
All endpoints MUST send keepalives for each data session. These
keepalives serve the purpose of keeping NAT bindings alive for the
data session. The keepalives SHOULD be sent using a format that is
supported by its peer. ICE endpoints allow for STUN-based keepalives
for UDP streams, and as such, STUN keepalives MUST be used when an
ICE agent is a full ICE implementation and is communicating with a
peer that supports ICE (lite or full).
https://datatracker.ietf.org/doc/html/rfc8445#section-11
from mediasoup.
It seems RFC7675 is not suitable for ICE-Lite.
But we need a way to detected unexpected closed clients.
Here is some similar issues in pion.
pion/ice#288
pion/webrtc#2045
from mediasoup.
We don't have a way to detect unexpected closed plain RTP clients. Such a thing doesn't even exist in RTP protocol. Why should we care about WebRTC (ICE) unexpected closures? This can be easily done at application level by implementing a keep alive mechanism in which the server sends "ping" messages over DataChannel to a client and the client must reply with "pong".
We should not invent things that do not exist in the RTC specs.
from mediasoup.
The only idea coming to my mind would be this:
- Once ICE+DTLS is established in a
WebRtcTransport
, mediasoup sends periodic DTLS messages to the client and expects a "response" in return. - But I don't think those DTLS messages exist in the DTLS spec and I don't even thing we can trigger them using OpenSSL.
from mediasoup.
Because WHEP and WHIP are using HTTP as protocol, and has not define keep-alive message. The all use RFC7675 to do this.
https://datatracker.ietf.org/doc/draft-murillo-whep/
Once a session is set up, ICE consent freshness [RFC7675] SHALL be used to detect abrupt disconnection and DTLS teardown for session termination by either side.
https://datatracker.ietf.org/doc/draft-ietf-wish-whip/
Once a session is setup, ICE consent freshness [RFC7675] SHALL be
used to detect non graceful disconnection and DTLS teardown for
session termination by either side.
from mediasoup.
Because WHEP and WHIP are using HTTP as protocol, and has not define keep-alive message. The all use RFC7675 to do this.
Yes, but mediasoup is ICE Lite so it cannot send ICE Binding requests to clients, and even if we sent them we couldn't reliably expect ICE answers from those clients since, as per spec, they are not mandated to reply to ICE requests received from ICE Lite endpoints (mediasoup).
from mediasoup.
@penguinol you want to see this :)
from mediasoup.
@ibc What is the benefit of implementing sending ICE Binding requests MS->clients instead of relying on missing ICE Binding requests clients->MS to detect when the connection is disconnected?
It looks like a more common approach and more efficient but maybe I'm missing something. (I have commented it with other people and shared the same comment/concern).
from mediasoup.
What is the benefit of implementing sending ICE Binding requests MS->clients instead of relying on missing ICE Binding requests clients->MS to detect when the connection is disconnected?
Client->MS periodic ICE consent mechanism is not mandatory or there could be ICE clients that do not implement it. We cannot assume that all clients implement it.
from mediasoup.
What is exactly wrong with being explicit and sending requests by ourselves? Or is it just that since there is an alternative legitimate approach then we have to undo the ongoing work?
from mediasoup.
The funny thing is that you are right and we could just run a timeout and report disconnection if no consent request was received in the last 30 seconds. HOWEVER the ongoing PR of this task has made a underlying bug to show up (maybe the server side timeout approach you suggest would also make it show up) so I want to fix that first. It's explained in the PR.
from mediasoup.
The argument I've seen for doing a STUN ping from server to client anyway is 💩 enterprise firewalls which let outgoing traffic through but block all incoming traffic.
from mediasoup.
The argument I've seen for doing a STUN ping from server to client anyway is 💩 enterprise firewalls which let outgoing traffic through but block all incoming traffic.
So that's a reason to rely on consent from client to server rather than from server to client, right? And I don't understand anyway. The NAT is open, otherwise there couldn't be incoming and outgoing ICE (requests or responses, DTLS, RTP, etc). So I don't get this argument at all.
And honestly I'm redoing this right now and it's being pain so I prefer no not discuss about this aeternum. I just assume that relying on consent requests sent by client is valid for ALL network scenarios.
from mediasoup.
No, that would be a reason to do what you proposed in addition to what @ggarber proposed. Such "enterprise" "firewalls" (quotes intentional!) are sometimes breaking enough stuff get get to a state where the DTLS handshake completes (over a different path which works in both directions such as TURN/UDP) but then the higher priority "direct" pair takes over sending from the client but the reverse channel is broken.
from mediasoup.
I promise I'm not gonna implement consent check in both directions (AKA sending server side requests plus checking received requests from clients).
from mediasoup.
Related Issues (20)
- Windows CI jobs failing due to meson HOT 4
- [Rust] Unable to build mediasoup > 0.12 with Docker HOT 3
- Issues in Node tests HOT 3
- Doubt with test HOT 6
- Worker hits ASSERT condition in production HOT 14
- About v3.13.15, io_uring, and liburing HOT 7
- [Rust] Worker panics when running with multiple workers HOT 19
- announced_ip can also be a hostname (not possible in Rust)
- Rename "announced ip" to "announced address"
- [Rust] `Box<dyn Error>` error types make mediasoup error handling annoying HOT 1
- Wrong handle of response in Channel.ts? HOT 1
- Fail to compile mediasoup fuzzer due to liburing HOT 21
- Fuzzer fails immediately due to abseil: AddressSanitizer: SEGV HOT 12
- (ABORT) RTC::RTCP::FeedbackRtpTransport::AddPacket() | failed assertion `baseSet': base not set HOT 4
- OpenSSL send buffer growing without bounds in DtlsTransport (worker memory leak) HOT 22
- Implement FLEX FEC - RFC 8627
- Memory leak HOT 6
- Multi-thread bug when using usrsctp in N Worker threads in Rust HOT 1
- opus dtx incorrectly judged HOT 7
- Possible missing break in H264_SVC::ParseSingleNalu()
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from mediasoup.