Don't fire "unregistered" event when there is no 200 to the un-REGISTER

[ibc]

(Reported by Fabian Santana in jssip maillist, and related to #25)

JsSIP emits "unregistered" event if it has no confirmation from the registrar (no 200 OK). Maybe jsSIP may fire other event, or perhaps provide some argument in the "unregistered" event telling the user whether the SIP un-registration succeded (200 OK) or not.

This maybe a change in the API so for 0.3.

[jmillan]

I think a new event 'unRegistrationFailure' will cover all the cases:

When executing unregister(), no event is emitted until response reception.
If possitive response is received, 'unregistered' event is fired.
If negative response is received, or timeout, or connection error happenns, 'unregistrationFailed' event is fired.

[saghul]

This behavior is not aligned with what we do for sessions, for example. In sessions we fire the ended event even if there is no 200 OK for the BYE. I'm not sure how relevant this is for registrations, in the end, you may unregister, and one second later the server sends you a call, what do you do?

Timeout may come very late, you don't want to be waiting that long for unregistering... I'd give the request a 1 second timeout, and if it's not replied within this amount of time, just fake the event. Nothing bad can happen anyway.

[jmillan]

Yes. If there is not response in the first second/s.., the response is not likely to reach anyway, and having to wait the transaction timeout makes the event unresponsive. This is quite a special case.

[jmillan]

Hi,

I guess the following approach satisfies all the needs:

• New event UA.registering. Fired when sending the registration request.
• Event UA.registered is fired as it is done now. Fired when response to registration request is positive.
• Event UA.registrationFailed is fired as it is done now. Fired when response to registration request is negative. or other failure ocurred.
• New event UA.unregistering. Fired when sending the unregistration request. now UA.unregistered is fired instead.
• Event UA.unregistered. Fired when response to unregistration request is positive.
• New event UA.unregistrationFailed. Fired when response to unregistration is negative or othe failure ocurred.

Every ed ending events come with the corresponding SIP response except those failure events which are fired due to other internal error, which cause is defined in the event itself.

[ibc]

+1

[saghul]

Why not have a single event: registrationStatusChanged and then in the payload have a status attribute with registrationStarted, registrationCompleted, registrationFailed, derefgistrationStarted, ....

[jmillan]

@saghul

It is a good approach indeed. This is something that may be extended to every JsSIP object that handles a 'status' attribute. It reduces the number of events in those objects handling a 'status' attribute like; UA, Registrator, Session.., etc.

This supposes a mayor change and is being considered.

Thanx for suggestion!

[jmillan]

Lets implement by adding those new events (registering, unregistering, unregistrationFailed) insead of the 'registrationStatusChanged' approach. Both are perfectly valid and usable. JsSIP does already implement the first approach in all the library.

[saghul]

That doesn't make it correct though ;-)

On Friday, February 1, 2013, José Luis Millán wrote:

Lets implement by adding those new events (registering, unregistering,
unregistrationFailed) insead of the 'registrationStatusChanged' approach.
Both are perfectly valid and usable. JsSIP does already implement the first
approach in all the library.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/26#issuecomment-12975550.

/Saúl
http://saghul.net | http://sipdoc.net

[jmillan]

I don' t find one option more correct than the other, just different.

Both are different (and not so much at the end) ways to fire events. The difference here is mostly cosmetic. Some could prefer one way over the other but I find this reason not heavy enough to make the change.

It would also imply an inconsistency between all other elements which state change and which named event implementation is OK from my point of view.

[saghul]

Fair enough. Even though my approach is a lot easier to extend, that it ,
to add new states if needed, since it doesn't change the public API.

On Fri, Feb 1, 2013 at 9:07 AM, José Luis Millán
notifications@gitpro.ttaallkk.topwrote:

I don' t find one option more correct than the other, just different.

Both are different (and not so much at the end) ways to fire events. The
difference here is mostly cosmetic. Some could prefer one way over the
other but I find this reason not heavy enough to make the change.

It would also imply an inconsistency between all other elements which
state change and which named event implementation is OK from my point of
view.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/26#issuecomment-12984430.

/Saúl
http://saghul.net | http://sipdoc.net

[ibc]

@saghul, with your approach the public API "would not change" but the user code would be broken or would not work as expected (since it did not include the new status "shitting" in the "if / else" stament).

Current approach is really good. Future possible changes will require changes, indeed, what's wrong? This is still 0.3.X version...

[saghul]

You didn't get my point. My point is that it's a much easier way to extend the API without breaking it. Reacting to any event has always been optional, so if yon't react to the new 'foobar' event, nothing happens. Now, if you would ever change the name of an event, that would be like changing the API.

[ibc]

So let's change the API and document it when needed. Or is the new fashion never to change the API?

[saghul]

Is not a new fashion, it's just a mechanism which makes it easier to manage
IMHO.

On Fri, Feb 1, 2013 at 10:57 AM, Iñaki Baz Castillo <
notifications@github.com> wrote:

So let's change the API and document it when needed. Or is the new fashion
never to change the API?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/26#issuecomment-12987635.

/Saúl
http://saghul.net | http://sipdoc.net

[jmillan]

Here I am. Back from the mud.

Apart of the event firing mechanism being discussed over the last comments in this issue, I don't feel like increasing the number of events/states makes it useful for the user. Though I made the first proposal.

As a API user, I wan't to know whether the UA is registered (Boolean). I want to get the response (if any) and cause for successful/unsuccessfull un/registration. But it is not easy for the user nor beautiful having 6 events/states for registration handling.

I find this proposal fine:

• Fire 'registered' event (with SIP response) only when receiving 200. As it is done now.
• Fire 'registrationFailure' event (with SIP response and cause*). As it is done now.
• Fire 'unregistered' event (with SIP response and cause*). Now it is fired before sending the un-REGISTER request.
• cause : May be a SIP response cause code or request timeout or transport failure.

The error with the actual implementation is that 'unregistered' is fired just before sending the un-REGISTER request. It must be fired:

• when receiving a SIP response to un-REGISTER requets. Regardless it is possitive or negative.
• If transaction timeout occurs
• If transport error ocurs.

In any of those three cases, the UA becames 'unregistered' and an event with such name is fired with the SIP response if any, and the cause, given by the SIP response, transaction tiemout or transport errror.

Note. Even if receiving a negative SIP response to un-REGISTER request, the UA is set as 'unregistered' and the 'unregistered' event is fired.

Any thoughts?

[ibc]

It's just perfect.

[saghul]

The naming is not consistent. registrationFailure should be registrationFailed, since the other events are actions, not nouns.

[jmillan]

Hi,

it was a typo en the issue comment. registrationFailed is the event name.

[saghul]

Great work!

On Sat, Feb 2, 2013 at 11:23 AM, José Luis Millán
notifications@gitpro.ttaallkk.topwrote:

Hi,

it was a typo en the issue comment. registrationFailed is the event name.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/26#issuecomment-13027447.

/Saúl
http://saghul.net | http://sipdoc.net