Don't chain error handler to success handler in Janus.httpAPICall in janus.js #2569

i8-pi · 2021-02-23T15:59:02Z

I ran into a problem in janus.js where multiple HTTP longpoll requests were running simultaneously. This was caused by the success handler and error handler in Janus.httpAPICall for longpoll both being called, which happened because the error handler was chained to the success handler, and the success handler threw an error from running a user supplied callback

This change separates the success handler from the error handler so they won't both be called

lminiero · 2021-03-02T10:11:10Z

Apologies for the delay of this response, we're quite busy on other activities. I can't remember if we ever encountered this issue, but we'll have a look in the next few days and let you know.

lminiero

Apologies again for this late response, we were quite busy with the IETF meeting.

Can you elaborate on what would be the impact of this change on existing applications? It's not clear to me what would happen in case the try/catch there fails: would it break the long poll loop? Since it's only started internally, there is no way for users to restart it externally when that happens. Thanks!

lminiero · 2021-03-16T11:25:37Z

html/janus.js

-						}).catch(function(error) {
+							try {
+								options.success(parsed);
+							} catch (error) {


Code style note: no space between catch and bracket.

i8-pi · 2021-03-16T18:05:56Z

This change should not break anyone's workflow, since there is an implicit assumption that only either the success or error callback is run.

In the case of longpoll, both the success and error callbacks trigger another longpoll request, so the existing behaviour of calling both is to slowly fork bomb your browser. createSession is another function where running both callbacks leaves you in an inconsistent state. Other places that call httpAPICall have error callbacks that effectively only log messages to the console. So the existing behavior is either broken or inconsequential.

If the try/catch fails, in the case of longpoll the next loop should still be triggered since it happens very early in the success callback and all the code up to that point is in janus.js (i.e. not user supplied callbacks)

lminiero · 2021-03-22T10:32:15Z

Ack, thanks for the clarification! We'll make a few tests and in case we notice no regression we'll merge 👍

lminiero · 2021-04-15T13:03:30Z

Apologies for the awful delay: I made a few tests, simulating errors in applications, and it seems to do the trick.
Merging then! 👍

* meetecho_master: (345 commits) Remove support for framemarking RTP extension (meetecho#2640) Prevent race conditions on socket close in SIP and NoSIP plugins (meetecho#2599) Fixed overflow runtime error Fix for race condition between VideoRoom publisher leaving and subscriber hanging up (fixes meetecho#2582) (meetecho#2637) Send PLI when starting a paused stream (meetecho#2645) Prevent too high shift exponent Fixed type of seq/ts in file-based Streaming mountpoint threads Fix missing g_thread_unref when a streaming helper thread quits. Added custom headers for SIP INFO request (meetecho#2644) Free participant->user_id_str in case of opus enc/decoder error. Reject flexfec when offered, as still unsupported (see meetecho#2639) Don't add rtx ssrc if m-line is recvonly/inactive (see meetecho#2639) Added NULL checks for json_dumps (see meetecho#2629) Parse custom headers, if required, in successful REGISTER response (fixes meetecho#2636) Timestamp correction for janus-pp-rec (meetecho#2573) Don't chain error handler to success handler in Janus.httpAPICall (meetecho#2569) Add missing library link for WS event handler (fixes meetecho#2628) Fixed broken switch in Streaming plugin when using helper threads Resolves meetecho#2624 jansson double referencing (meetecho#2634) Unlock mountpoints mutex after the spawning of helper threads. ... # Conflicts: # transports/janus_mqtt.c

* meetecho_master: (345 commits) Remove support for framemarking RTP extension (meetecho#2640) Prevent race conditions on socket close in SIP and NoSIP plugins (meetecho#2599) Fixed overflow runtime error Fix for race condition between VideoRoom publisher leaving and subscriber hanging up (fixes meetecho#2582) (meetecho#2637) Send PLI when starting a paused stream (meetecho#2645) Prevent too high shift exponent Fixed type of seq/ts in file-based Streaming mountpoint threads Fix missing g_thread_unref when a streaming helper thread quits. Added custom headers for SIP INFO request (meetecho#2644) Free participant->user_id_str in case of opus enc/decoder error. Reject flexfec when offered, as still unsupported (see meetecho#2639) Don't add rtx ssrc if m-line is recvonly/inactive (see meetecho#2639) Added NULL checks for json_dumps (see meetecho#2629) Parse custom headers, if required, in successful REGISTER response (fixes meetecho#2636) Timestamp correction for janus-pp-rec (meetecho#2573) Don't chain error handler to success handler in Janus.httpAPICall (meetecho#2569) Add missing library link for WS event handler (fixes meetecho#2628) Fixed broken switch in Streaming plugin when using helper threads Resolves meetecho#2624 jansson double referencing (meetecho#2634) Unlock mountpoints mutex after the spawning of helper threads. ... # Conflicts: # transports/janus_mqtt.c (cherry picked from commit 1827315)

* meetecho_master: (345 commits) Remove support for framemarking RTP extension (meetecho#2640) Prevent race conditions on socket close in SIP and NoSIP plugins (meetecho#2599) Fixed overflow runtime error Fix for race condition between VideoRoom publisher leaving and subscriber hanging up (fixes meetecho#2582) (meetecho#2637) Send PLI when starting a paused stream (meetecho#2645) Prevent too high shift exponent Fixed type of seq/ts in file-based Streaming mountpoint threads Fix missing g_thread_unref when a streaming helper thread quits. Added custom headers for SIP INFO request (meetecho#2644) Free participant->user_id_str in case of opus enc/decoder error. Reject flexfec when offered, as still unsupported (see meetecho#2639) Don't add rtx ssrc if m-line is recvonly/inactive (see meetecho#2639) Added NULL checks for json_dumps (see meetecho#2629) Parse custom headers, if required, in successful REGISTER response (fixes meetecho#2636) Timestamp correction for janus-pp-rec (meetecho#2573) Don't chain error handler to success handler in Janus.httpAPICall (meetecho#2569) Add missing library link for WS event handler (fixes meetecho#2628) Fixed broken switch in Streaming plugin when using helper threads Resolves meetecho#2624 jansson double referencing (meetecho#2634) Unlock mountpoints mutex after the spawning of helper threads. ... # Conflicts: # transports/janus_mqtt.c (cherry picked from commit 1827315) (cherry picked from commit b9318b7)

lminiero reviewed Mar 16, 2021

View reviewed changes

Don't chain error handler to success handler in Janus.httpAPICall

23c52b6

i8-pi force-pushed the isolate-httpapicall-success branch from bf7ce15 to 23c52b6 Compare March 16, 2021 17:59

lminiero merged commit fee03d4 into meetecho:master Apr 15, 2021

i8-pi deleted the isolate-httpapicall-success branch January 2, 2022 03:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't chain error handler to success handler in Janus.httpAPICall in janus.js #2569

Don't chain error handler to success handler in Janus.httpAPICall in janus.js #2569

i8-pi commented Feb 23, 2021

lminiero commented Mar 2, 2021

lminiero left a comment

lminiero Mar 16, 2021

i8-pi commented Mar 16, 2021

lminiero commented Mar 22, 2021

lminiero commented Apr 15, 2021

Don't chain error handler to success handler in Janus.httpAPICall in janus.js #2569

Don't chain error handler to success handler in Janus.httpAPICall in janus.js #2569

Conversation

i8-pi commented Feb 23, 2021

lminiero commented Mar 2, 2021

lminiero left a comment

Choose a reason for hiding this comment

lminiero Mar 16, 2021

Choose a reason for hiding this comment

i8-pi commented Mar 16, 2021

lminiero commented Mar 22, 2021

lminiero commented Apr 15, 2021