Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

api for sessions #68

Closed
vf1 opened this issue Feb 27, 2013 · 45 comments
Closed

api for sessions #68

vf1 opened this issue Feb 27, 2013 · 45 comments

Comments

@vf1
Copy link

vf1 commented Feb 27, 2013

It would be nice to have api for sessions: enumerate, get by uri etc. Because application should have sessions list almost always, probably there is no reason to duplicate this functionality.

@ibc
Copy link
Member

ibc commented Feb 27, 2013

I don't strongly agree. JsSIP wants to be a lowlevel library. The application on top of it should manage "sessions" instead.

Typicall the application using JsSIP retrieves the Session object in the UA.on("newSession") event (for both incoming and outgoing sessions) so the application can handle them and provide callbacks for removing them from the application's sessions collection and from the HTML dom (if required).

@saghul
Copy link
Contributor

saghul commented Feb 27, 2013

Applications can inspect ua.sessions, it contains all the current sessions. I also don't see a need to getting by URI or what not.

@vf1
Copy link
Author

vf1 commented Feb 27, 2013

Yes, I found ua.sessions, but it is not fair... :)

@ibc
Copy link
Member

ibc commented Feb 27, 2013

ua.sessions is not part of the public API and could change in any moment. IMHO a well designed application on top of JsSIP should not need retrieving the list of sessions from JsSIP at all.

@vf1
Copy link
Author

vf1 commented Feb 27, 2013

I do not see any difference between ua.sessions and application.sessions? Why each developer will maintain own list if it is already implemented in library.

@ibc
Copy link
Member

ibc commented Feb 27, 2013

Let's leave this issue open for studying whether us.sessions should be part of the public API or not.

@saghul
Copy link
Contributor

saghul commented Feb 27, 2013

I think it should. But should it return a copy or de read only, in case
someone messes with it?

On Wednesday, February 27, 2013, Iñaki Baz Castillo wrote:

Let's leave this issue open for studying whether us.sessions should be
part of the public API or not.


Reply to this email directly or view it on GitHubhttps://github.com//issues/68#issuecomment-14174598
.

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

@ibc
Copy link
Member

ibc commented Feb 28, 2013

It's not possible to provide a "read only" or a "copy" of ua.sessions. Each session has lot of attributes containing other objects and so on. And, it's supposed that the developer maybe interested in inspecting ua.sessions for terminating all the sessions or whatever.

If ua.sessions becomes part of the public API, and the developer messes with it, then that is the developer's problem.

@saghul
Copy link
Contributor

saghul commented Feb 28, 2013

A copy of the array, not the sessions themselves. But you are right, we can't prevent the user from shutting himself in the foot if he wants to.

@ibc
Copy link
Member

ibc commented Mar 19, 2013

@jmillan should @ua.sessions@ be part of the public API? If so we should document it in the API. However I'm not sure...

@jmillan
Copy link
Member

jmillan commented Mar 19, 2013

Since the high layer app is already noticed about every new Session via the corresponding event, it can store them following the most suitable pattern in each case when convenient, for later access. That is meant to be part of each app logic.

I don't feel like JsSIP should give a high layer API for retrieving Sessions.

@saghul
Copy link
Contributor

saghul commented Mar 19, 2013

What about that use case when a user reloads a window while there are active calls? Why should we force the user to keep track of sessions if we are already doing it?

@ibc
Copy link
Member

ibc commented Mar 19, 2013

IMHO the only question here is: make @ua.sessions@ API public or not. I don't think that should be painful. Agree?

@jmillan
Copy link
Member

jmillan commented Mar 19, 2013

Making ua.sessions public is not painful, neither useful right now. I was speaking about offering an API for retrieving the Sessions, which is what @vf1 was asking for, if I understood ok.

@jmillan
Copy link
Member

jmillan commented Mar 19, 2013

@saghul, a window reload will tear down any UA instance as well.

@ibc
Copy link
Member

ibc commented Mar 19, 2013

Yep, I strongly discourage the API for retrieving sessions requested by @vf1, I just mean the @ua.session@ method.

@saghul
Copy link
Contributor

saghul commented Mar 19, 2013

Yep, I strongly discourage the API for retrieving sessions requested by @vf1, I just mean the @ua.session@ method.

Agreed.

@jmillan at least you could popup an alert panel or something.

@jmillan
Copy link
Member

jmillan commented Mar 19, 2013

Yes, but that has nothing to see with JsSIP, has it? It is managed via the window.onunload event, which you use to alert web user about the imminent information lost.

If you feel that making ua.sessions public is useful, ok. I think that if an app needs to look for calls there, it is not well designed. I may be wrong.

Some use cases sustaining the approach are welcome though.

@saghul
Copy link
Contributor

saghul commented Mar 19, 2013

On 3/19/13 6:28 PM, José Luis Millán wrote:

Yes, but that has nothing to see with JsSIP, has it? It is managed via
the |window.onunload| event, which you use to alert web user about the
imminent information lost.

If you feel that making |ua.sessions| public is useful, ok. I think that
if an app needs to look for /calls/ there, it is not well designed. I
may be wrong.

Some use cases sustaining the approach are welcome though.

I want to send an in-dialog SIP MESSAGE to everyone I'm talking to
saying "be right back" on window reload. How do I do it without having a
list of sessions? Or tear them down properly by sending a BYE.

Saúl Ibarra Corretgé
http://about.me/saghul | http://saghul.net

@ibc
Copy link
Member

ibc commented Mar 19, 2013

IMHO JsSIP is a core library so it's responsability of the app on top of JsSIP storing and managing sessions (which are given via the ua.newSession event). The application on top of JsSIP should store each RTCSession instance into an array or collection and remove it when it fires the "failed" or "ended" event.

@jmillan
Copy link
Member

jmillan commented Mar 19, 2013

None prevents you from storing the Sessions as you get them via ua. newRTCSession event, at your needs. I'm sure that you will need to store them anywhere in your app in order to manage them.., there could be some exception for sure.

@ibc
Copy link
Member

ibc commented Mar 19, 2013

IMHO we can close this issue. ua.sessions is "public" for now (regardless it's not documented), but we don't agree on the need on having it as "real public API method" since JsSIP is not an application but a library.

@saghul
Copy link
Contributor

saghul commented Mar 19, 2013

None prevents you from storing the Sessions as you get them via ua. newRTCSession event, at your needs. I'm sure that you will need to store them anywhere in your app in order to manage them.., there could be some exception for sure.

I know it can be done like that, but why do you insist in having the user duplicate the work already done by the library?

IMHO we can close this issue. ua.sessions is "public" for now (regardless it's not documented), but we don't agree on the need on having it as "real public API method" since JsSIP is not an application but a library.

No, if it's public it has to be documented as such, otherwise someone may rely on it and will be pissed off if we change it.

I doesn't require any effort on our end to say it's public and we (potentially) add some value. I fail to see why you are against it and want to force the user to do the same thing which the library already does. FWIW, we have the same thing in python-sipsimple and it's part of the public API.

@ibc
Copy link
Member

ibc commented Mar 20, 2013

Is it a Pandora'x box? If we let ua.sessions to be public API, may be we will also be requested to let ua.registrator or ua.applicants (non-INVITE transaction apps like Message) as part of the public API?

I strongly think that using ua.sessions is a bad design of the app on top of JsSIP.

The developer should save the session obtained in newRTCSession event in an array/object within him application on top of JsSIP (if he needs it).

@saghul
Copy link
Contributor

saghul commented Mar 20, 2013

I still haven't seen a single reason explaining why it's such a "bad" idea. Users can make calls, why not give them the damm list of calls?! Seriously.

Anyway, do whatever you want, I'm tired of this bikeshed discussion.

@ibc
Copy link
Member

ibc commented Mar 20, 2013

The developer can call to ua.close() when detecting that the browser/tab is being closed (window.unload event) to automatically terminate all the sessions. If the developer wants to send a MESSAGE to the remote peer of all the active sessions then he should store the sessions at his convenience.

ua.sessions currently stores JsSIP.RTCSession instances. In the future it could be renamed to ua.rtcSessions or whatever (think in MSRP or datachannel based sessions).

JsSIP is not the app but the library.

@ibc
Copy link
Member

ibc commented Dec 9, 2013

Let's close this issue. After implementing several apps on top of JsSIP I strongly consider that retrieving the list of active sessions from JsSIP itself means a bad programming practice. Really, things must not be done in that way.

If the user wants to terminate all the active sessions at a given time (i.e. when the browser tab is to be closed and the window event unload is fired) then it can safely call to UA.stop():

http://jssip.net/documentation/devel/api/ua/#method_stop

@ibc ibc closed this as completed Dec 9, 2013
@saghul
Copy link
Contributor

saghul commented Dec 9, 2013

I still disagree.

@ibc
Copy link
Member

ibc commented Dec 9, 2013

Why? I would repeat my comment:

After implementing several apps on top of JsSIP I strongly consider that retrieving the list of active sessions from JsSIP itself means a bad programming practice. Really, things must not be done in that way.

Really, it somebody needs to get the active sessions from JsSIP itself then it is doing things wrongly.

@saghul
Copy link
Contributor

saghul commented Dec 9, 2013

The fact that you didn't need it doesn't mean it's not useful.

@ibc
Copy link
Member

ibc commented Dec 9, 2013

Tell me one usecase.

@ibc ibc reopened this Dec 9, 2013
@saghul
Copy link
Contributor

saghul commented Dec 9, 2013

#68 (comment)

But you said that:

If the developer wants to send a MESSAGE to the remote peer of all the active sessions then he should store the sessions at his convenience.

So, why do you ask me if you don't want to add it anyway?

Anyhow, I would make that sessions array "private" (_sessions name) and add an API function like getActiveSessions, which returns a copy of it, thus preventing the user from breaking things.

@ibc
Copy link
Member

ibc commented Dec 9, 2013

We don't use "_xxxxx" Pythonist stuff for naming private methods or attributes, but I am OK with having something like ua.getActiveSessions() (althought I consider that a bad programming practice).

@saghul
Copy link
Contributor

saghul commented Dec 9, 2013

It's used in Python yes, but also in Node, that's why I suggested it, I don't know what the convention is.

You never explained why it's a "bad programming practice". It's not. I gave you good enough reasons 9 months ago, so do whatever you want I won't lose more time with this bikeshed discussion.

@ibc
Copy link
Member

ibc commented Dec 9, 2013

Let's be clear. Do you know why some people have requested this feature? To terminate active calls when the browser tab is closed. Good news: we have ua.stop().

@ibc ibc closed this as completed Mar 11, 2014
@vf1
Copy link
Author

vf1 commented Mar 11, 2014

I am using list of session for sending special message to all participants on some events in https://github.com/vf1/muvconf. It peer-to-peer multi-user chat, and conference is maintained by clients w/o server.

@ibc
Copy link
Member

ibc commented Mar 11, 2014

IHMO you should keep the list of participants rather than the list of "JsSIP RTPSession's" and, anyhow, if you use JsSIP.UA.sessions then you are limited to a single multiconference per JsSIP::UA instance.

As said above, IMHO that is not a good design. It can work, but it is not a good design, and, as developer I cannot suggest such a way to refer to all the active sessions.

@vf1
Copy link
Author

vf1 commented Mar 12, 2014

I disagree with your opinion about design. It is real example, and it is not relayted to ua.stop() at all.

@saghul
Copy link
Contributor

saghul commented Mar 12, 2014

FWIW, I agree with @vf1. UA.sessions should be considered part of the API. Anyway, it's there so you can use it, despite @ibc's stubbornness.

@ibc
Copy link
Member

ibc commented Mar 12, 2014

JsSIP provides (along with many others) these events:

  • A session is generated (and provide the RTCSession instance).
  • A session is established.
  • A session is ended (terminated, failed, canceled).

May I understand what else is needed for the user to keep the list of existing sessions?

@saghul
Copy link
Contributor

saghul commented Mar 12, 2014

Nothing. jsSIP already keeps it. Stop saying it's not ok to use it. Period.

@ibc
Copy link
Member

ibc commented Mar 12, 2014

It's not ok to use it. Reasons:

  1. It is not a public API so it may change (unless this feature request is accepted).
  2. Nowhere is written for how long JsSIP keeps a session there (in ua.sessions). Do you assume that a session is removed from there when the session terminates? what happens if JsSIP (today or tomorrow) decides to keep the RTCSession there for a bit more when waiting for the 200 for a sent BYE? Note that RTCSession.terminate() and most of the other methods throw a INVALID_STATE_ERROR exception if called when the session is already terminated.
  3. Coming back to point 2, the RTCSession API does not provide a method to get the current "status" of a session and thus there is no way for the user to know whether it can invoke methods on a RTCSession or not.

So, accepting this feature request (make ua.sessions public API) means also provide an API mehtod to get the status of a RTCSession, which means also exposing internal constants to the API.

@saghul hope you find above some reasons not to insist on this topic (at least as it is requested now).

@saghul
Copy link
Contributor

saghul commented Mar 12, 2014

It is not a public API so it may change (unless this feature request is accepted).

This is just circular reasoning. "We don't accept it because it's private and it's private because we don't accept it.". Seriously.

Nowhere is written for how long JsSIP keeps a session there (in ua.sessions). Do you assume that a session is removed from there when the session terminates? what happens if JsSIP (today or tomorrow) decides to keep the RTCSession there for a bit more when waiting for the 200 for a sent BYE? Note that RTCSession.terminate() and most of the other methods throw a INVALID_STATE_ERROR exception if called when the session is already terminated.

The user can check the state.

Coming back to point 2, the RTCSession API does not provide a method to get the current "status" of a session and thus there is no way for the user to know whether it can invoke methods on a RTCSession or not.

Then expose the state as well.

So, accepting this feature request (make ua.sessions public API) means also provide an API mehtod to get the status of a RTCSession, which means also exposing internal constants to the API.

And what is the problem? Instead of exposing constants just provide a human readable status like 'connecting' 'connected', 'ending', 'ended' etc.

@saghul hope you find above some reasons not to insist on this topic (at least as it is requested now).

They are insufficient. If you don't want to add this, they say the real reason: you don't want to. Don't try to continue arguing about it when you've already been proven wrong.

@ibc
Copy link
Member

ibc commented Mar 12, 2014

I don't want to enforce bad programming practices. Still I haven't seen a real usecase in which iterating the ua.sessions list is the best choice.

@saghul
Copy link
Contributor

saghul commented Mar 12, 2014

The fact that it's a bad programming practice it's your opinion. And you are wrong. I gave you an example ages ago and @vf1 gave you a very valid use case.

Just stop it already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants