choose UPDATE/RE-INVITE on sending session refresh request #384
Conversation
as RFC 4028 refresher can send re-INVITE or UPDATE, introduce JsSIP SIP User Agent parameters (session_timers_use_update) to enable/disable UPDATE in session refresh request
|
There is a good reason for usunge UPDATE for session timers rather then reINVITE: The purpose of session timers is to keep the signaling open and be able to check it. Using UPDATE for that is perfect. In the other side, using reINVITE means renegotiating the SDP session (because the INVITE/200/ACK must handle a SDP Offer/Answer) which is 100% unnecessary if the only we need is to check the session status. Also, your pull request makes reINVITE the default session timers mechanism, which changes the current default settings. I'm sorry but I strongly consider that using UPDATE for session timers is the proper way to go and, given that we are in 2016, I hope SIP servers, proxies and endpoints MUST implement the UPDATE method (defined in RFC 3311 in 2002, 14 years ago). |
|
I agree with you @ibc that is better of use UPDATE rather reINVITE but the purpose of RFC is to define an Internet Standard, and in software engineering follow standard was the goal for every developer. If you write that jsSIP support RFC 4028 this mean that i dont trouble to check that all thing of this RFC was developed (or you must specify what is or not is present).
Absolutely no, check again the code, new parameter name was
and default value is true. This mean that default session timers mechanism remaining UPDATE |
|
OK, let us recheck it during this week. |
|
@jmillan any thoughts on this? |
|
I'm ok with this PR. 👍 |
There was a problem hiding this comment.
@michelepra please fix the issues detected in the review, and run gulp lint to check that the code style is correct.
lib/RTCSession.js
Outdated
| var expires = this.sessionTimers.currentExpires; | ||
| var self = this, | ||
| expires = this.sessionTimers.currentExpires, | ||
| eventHandlers; | ||
|
|
There was a problem hiding this comment.
Please, put each variable declaration in a single line var xxx = xxx;.
lib/RTCSession.js
Outdated
| succeeded: function (response) { | ||
| handleSessionTimersInIncomingResponse.call(self, response); | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
Here there is a mix of tabs and spaces. Please, use two spaces for indentation.
lib/RTCSession.js
Outdated
| sendReinvite.call(self, { | ||
| eventHandlers: eventHandlers | ||
| }); | ||
| } |
There was a problem hiding this comment.
Same here, mix of tabs and spaces. Use two spaces, please.
Modify code as requested in versatica#384
|
Many time passed from last review, now i correct file as requested |
|
+1 |
Decouple signaling and WebRTC stuff versatica#427 rpid parser add async modifier for local and remote description add option to use reinvite instead update for session timer
|
Hi @michelepra, Would you create a new clean PR out of current master? I could do it otherwise. |
|
i will create new PR next week. Before i must generate new clone of master because my branch now include fix of #427 and other minor change (it would not be bad idea if is integrated into jssip master) |
|
Thanks, no hurries. We would honestly like to do the decoupling ourselves. We have now more experience than we had before and more clear ideas on how to do it. |
|
@michelepra, for me it would be easy to do the port: just in case you don't want to mess up with it. |
|
ok, sorry @jmillan this week i'm extremly full, no time to work on this. If you want close this pr and do the port, for me nothing change. Very interesting thing in my branch is decoupling, that work very well in production with nodejs enviroment, but i'd like someday to use official code to do same thing. Any plan when release? |
|
Landed in 3.1.0. Thanks! |
as RFC 4028 refresher can send re-INVITE or UPDATE.
Introduce JsSIP SIP
User Agent parameters (session_timers_use_update) to enable/disable
UPDATE in session refresh request