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

Waiting for candidates-done callback #126

Closed Will-I4M closed 9 years ago

Will-I4M commented 9 years ago

Hi ! First of all, thanks for the great job you did on Janus, it is a very interesting project. I notice that sometimes, if client connect/disconnect quickly without following the entiere neg. process, Janus can be trapped in an infinite loop :

[139824874914120] Waiting for candidates-done callback... [139824874914120] Waiting for candidates-done callback... [139824874914120] Waiting for candidates-done callback... [139824874914120] Waiting for candidates-done callback... [139824874914120] Waiting for candidates-done callback... [139824874914120] Waiting for candidates-done callback...

in janus.c line 3250 -> while ..... maybe adding a counter or something like that will fix this issue. eg

int max_time = 0 ; 
        while(ice_handle->cdone < ice_handle->streams_num && (max_time < SESSION_TIMEOUT/10) ) 
        {
            JANUS_LOG(LOG_INFO, "[%"SCNu64"] Waiting for candidates-done callback...\n", ice_handle->handle_id);
            g_usleep(100000);
            if(ice_handle->cdone < 0) {
                JANUS_LOG(LOG_ERR, "[%"SCNu64"] Error gathering candidates!\n", ice_handle->handle_id);
                return NULL;
            }
            max_time++; 
        }

not tested yet.

lminiero commented 9 years ago

Good catch. A fixed timeout will probably not help there, though, since the gathering of candidates may indeed take some more time, e.g., if you're using STUN servers on the server side as well, or if you have some particularly slow network interfaces. Since we're planning to allow Janus to use TURN relays as well (that is, not only for clients but also for Janus itself, although it should never actually be really needed) that could make a static timeout even less reliable.

What probably needs to be done is checking whether or not the handle or ongoing negotiation is still valid in the while, and give up if it's not.

Will-I4M commented 9 years ago

Thanks for your reply

i'm not sure if janus_plugin_sessions are stored in the janus_session 'table', or if sessions are removed from the hashtable, for now I'm exploring your code...

I'll try :

[janus.c L.3249]

while(ice_handle->cdone < ice_handle->streams_num && janus_session_find(((janus_session *)((janus_ice_handle *)handle->gateway_handle)->session)->session_id )) 

OR

while(ice_handle->cdone < ice_handle->streams_num && !((janus_session *)((janus_ice_handle *)handle->gateway_handle)->session->destroyed ) 

Do you plan to add a check on the handle in the while soon ? Can I use janus_session_find to check if the session is still valid ?

Sorry if i'm wasting your time, i need to understand your code in order to write a plugin for my needs.

WM

lminiero commented 9 years ago

Yes I'll work on a fix, as soon as I get back to work after the holidays. I'll probably start working from home either tomorrow or the day after that, so before I get to the lab.

Will-I4M commented 9 years ago

thanks a lot !

I discovered some minor typo in the videoroom plugin, could I build a list and send it to you by mail ?

lminiero commented 9 years ago

You can apply them in one of your branches and then ask for a pull request, this will have your changes appear on github where we can review this together.

lminiero commented 9 years ago

About the candidates-done thing, can you let me know if this solves it for you?

Will-I4M commented 9 years ago

Sorry for the delay, I tried the first solution in my previous post. This solution gave me good results, janus stay in the loop until the session close, not ad infinitum. Your fix seems to be a far better solution. The behaviour is similar, janus stay in the loop until

"[170691599] ICE send thread leaving.. " Detaching handle from JANUS VideoRoom plugin No WebRTC media anymore [....] Handle detached (0), scheduling destruction Detaching handle from JANUS VideoRoom plugin No WebRTC media anymore

Detaching handle from JANUS VideoRoom plugin No WebRTC media anymore Handle detached (0), scheduling destruction [WARN] [170691599] Handle detached or PC closed, giving up...!

Thanks again for your work on this fix !

Will-I4M commented 9 years ago

If I freeze network communications on client-side once janus is waiting in the loop instead of closing sessions, i get (about one minute after) :

[ERR] [janus.c:janus_handle_sdp:3263:] [287531737] Error gathering candidates! [ERR] [janus.c:janus_push_event:3133:] [287531737] Cannot push event (handle not available anymore or negotiation stopped)

and janus get out of the loop. Perfect :)

lminiero commented 9 years ago

Yes we have a one minute timeout for sessions in Janus. Once that expires, the session and its handles are destroyed.

L.

2015-01-05 15:32 GMT+01:00 Will-I4M notifications@github.com:

If I freeze network communications on client-side once janus is waiting in the loop instead of closing sessions, i get (about one minute after) :

[ERR] [janus.c:janus_handle_sdp:3263:] [287531737] Error gathering candidates! [ERR] [janus.c:janus_push_event:3133:] [287531737] Cannot push event (handle not available anymore or negotiation stopped)

and janus get out of the loop. Perfect :)

— Reply to this email directly or view it on GitHub https://github.com/meetecho/janus-gateway/issues/126#issuecomment-68714804 .