meetecho / janus-gateway

Janus WebRTC Server
https://janus.conf.meetecho.com
GNU General Public License v3.0
8.15k stars 2.47k forks source link

Streaming plugin hangs if peer connection is cutted #699

Closed Triuman closed 7 years ago

Triuman commented 7 years ago

As soon as Janus and browser start negotiating, I refresh the browser, which cuts the negotiation process. Just before dying, the browser sends a "stop" request to Janus but even Janus gets the message, it ignores it and wait for browser to connect it via webrtc. You can see the line that keeps Janus from stopping the negotiation process and free the webrtc resources. When I deleted "|| !session->started" part, Janus worked as expected and freed the resources.

https://github.com/meetecho/janus-gateway/blob/master/plugins/janus_streaming.c#L2249

If that makes sense, would you mind doing the necessary change?

Thank you.

lminiero commented 7 years ago

I'd rather keep the check there: we probably need to add some more state, as started assumed a negotiation has been completed, but as you said we might want to hangup a PeerConnection that was attempted but never finished. Maybe adding a new value that defines this state is a better way to handle that.

Anyway, I don't think you're describing the problem you're experiencing correctly. The Streaming plugin definitely doesn't "hang" when that happens: it keeps on doing what it does, and serving old and new users. What is left dangling is just the pending PeerConnection between Janus and the user who abruptly went away. Even doing nothing, the session is eventually destroyed (we have timeouts in place) and so those WebRTC resources are removed within a minute anyway.

If the problem you're experiencing is different, please try and describe better what is happening.

Triuman commented 7 years ago

Sorry for misunderstanding. But you are right, it keeps working. I'm using a slightly modified version of Janus and using the same session for different browser connections one after another. When one of them cause problem, others cannot request a watch using the same session.

Anyways, the problem is as you mentioned and I did the necessary change on my branch. Feel free to keep this open or close. Thank you for your prompt reply.

NOTE: I can also change the title to a more relevant one if you want this open for later.

lminiero commented 7 years ago

Not sure we actually need an issue open, as I don't see an issue occurring here. As I wrote, if you didn't close the session cleanly (no detach handle and/or destroy session) then Janus has a timeout mechanism in place to get rid of the related PeerConnection(s) by itself after a while. Unless I'm misunderstanding something?

Triuman commented 7 years ago

In my case a there is a websocket server between Janus and the browser. So, even if browser quits the page, the server keeps sending 'keepalive' messages. This is a very different usage than Janus is designed but still I believe Janus needs to 'free webrtc resources' to support modified usages. Someone would want to disable 'keepalive' messages later on. Then they may face this problem.

lminiero commented 7 years ago

It's not an unusual usage, we make use of Janus API wrappers a lot ourselves. Send a "hangup" and/or a "detach" request on the handle and the PeerConnection is removed: of course detaching is only required when the handle is not needed anymore. Doing that as a consequence of a user going away is your responsibility, as since you're wrapping the API Janus doesn't have any way of figuring out the user is gone by looking at the signalling or connections going away (it always talks to your server).