Subscribe support #711

ikq · 2021-05-20T15:21:07Z

I added Subscriber.js Notifier.js to JsSIP

I tried write them according JsSIP style and lint errors.
Please take a look and let me know what needs to be fixed.

I already see that *.d.ts files are missing.

The work still in progress: I want add to SUBSCRIBE Contact +sip.instance
as you do in REGISTER

jmillan · 2021-05-21T05:57:27Z

JsSIP code must have the same style in order to make it readable and maintainable. Comments before making a deeper review:

Debug line as the first method call in al public method implementations.
New line after return.
New line before and after call to super().
First conditional in a block starts in new line and last ends in a new line.
Conditional blocks always contain curly brackets.
Comments in the line above when possible.
Calls to debug have usually a new line above and a new line below.

Please take a certain file (ie: RTCSession.js) as a reference and try to stick to its style.

jmillan · 2021-05-25T06:25:11Z

@ikq

Thanks for the cleanup, 👍 We'll review the PR as we can.

jmillan · 2021-05-27T05:55:46Z

@ikq,

Cosmetic comment, we use camelCase in API method arguments (not in 100% of the cases, I know, but we will get there). Could you please make such change?

jmillan

I've made the first review.

Please go through all the comments and modify as needed. We'll go for sencond review afterwards.

lib/Notifier.js

lib/Subscriber.js

ikq · 2021-06-05T11:56:25Z

I mostly fixed the issues.

[I thought about dialog:
if you write in C++/java style, then you could add an abstract class dialog, and invite-dialog, subscriber and notifier dialogs would be his descendants.
The abstract dialog class could be something like stripped version of Dialog.js without actual method implementations]

About dialog, seems:

dialog interface:

id (string)
state (number)
receiveRequest receive incoming request with the id

other functionality:

check incoming requests
keep set of parameters to create outgoing request.

It's already implemented in Subscriber/Notifier.
[Not exactly as in Dialog.js, but the same functionality]

jmillan · 2021-06-08T12:58:22Z

Hi,

Super busy lately. Will come back to this when we have some time.

jmillan · 2021-06-22T14:11:27Z

H @ikq,

Dialog should be agnostic to the method. Why can't current Dialog implementation be used here? It has basically the sendRequest() method that you can call, and it will call the receiveRequest() method on your Subscriber/Notifier implementation.

ikq · 2021-06-22T15:03:25Z

Hmm...
I checked the Dialog.js code again.
sendRequest and receiveRequest can be used.

I just did not consider this possibility before because I found INVITE/CANCEL/ACK specific code and properties in the Dialog (e.g. subscriber used different dialog states set).

jmillan · 2021-06-22T16:21:53Z

There is INVITE related code in Dialog because a dialog for INVITE method needs to behave in certain way, different to generic dialogs, but that's an implementation detail. Other clases should be able to create dialogs the same way RTCSession does.

ikq · 2021-06-24T18:41:59Z

Hi Jose,
Now subscriber and notifier used Dialog.js
It passed my basic testing now.

I'll continue to test next week:

I cannot check subscriber authentication (my sip server don't request it).
subscriber with some production server.
subscriber with notifier.

ikq · 2021-07-07T15:09:43Z

Testing of the current version has been completed successfully.
(subscribe authentication was not checked)

ikq · 2021-07-10T13:17:00Z

subscribe authentication checked using special test.
IMHO: the subscriber/notifier is ready.

ikq · 2021-08-08T10:34:11Z

I see I should change debug/debugerror to logger...

jmillan · 2021-08-18T17:30:44Z

Hi @ikq,

We are super busy lately. Please keep using your branch and update this PR whenever you find any issue/enhancement.

Also, tests would accelerate the adoption.

ikq · 2021-08-20T07:37:14Z

I already use this code for my work and so far no problem.

Two browsers test can be taken from:
https://github.com/ikq/subscribe_notify_test

jmillan · 2021-08-23T09:38:19Z

@ikq, more than an external test I mean tests in JsSIP as part of this PR, testing the clases created here.

N4COM · 2021-09-22T09:22:52Z

Hi, is there some news? I'd like to test this functionality.

ikq · 2021-09-22T10:32:04Z

Hi, is there some news? I'd like to test this functionality.

IMHO it's stable code.
[At least we use subscriber in our production, currently no issues reported]

The subscriber/notifier test can taken from https://github.com/ikq/subscribe_notify_test

markusatm · 2021-09-22T10:49:13Z

Hi, is there a specific reason why Subscriber and Notifier don't use the Contact header from the UA like RTCSession does? If the user provides a custom contact_uri in the UAConfiguration he would have to manually add it as a extra header to Subscriber and Notifier again, wouldn't he?

N4COM · 2021-09-22T10:57:47Z

Hi, is there some news? I'd like to test this functionality.

IMHO it's stable code.
[At least we use subscriber in our production, currently no issues reported]

The subscriber/notifier test can taken from https://github.com/ikq/subscribe_notify_test

I'd like to use the official repository, so I'd like if it will be merged ASAP

ikq · 2021-09-22T11:28:26Z

Hi, is there a specific reason why Subscriber and Notifier don't use the Contact header from the UA like RTCSession does? If the user provides a custom contact_uri in the UAConfiguration he would have to manually add it as a extra header to Subscriber and Notifier again, wouldn't he?

Yes, I'm not using the Configuration.contact_uri

In fact, I was in a hurry to do this work, and did not pay attention at all to that there is such a parameter in the configuration !

Let check, how the configuration used for INVITE:

UA.js:
   this._contact = {
      pub_gruu  : null,
      temp_gruu : null,
      uri       : this._configuration.contact_uri,

RTCSession.js:
  this._contact = this._ua.contact.toString({
      anonymous,
      outbound : true
    });

Thanks.
It can be improved (use of Configuration.contact_uri)

P.S.
Usage Configuration.contact_uri will modify default SUBSCRIBE contact - will be missed "+sip.instance" (that I add to default SUBSCRIBE Contact)
User still can set "+sip.instance" to SUBSCRIBE Contact via custom Contact header.

mitre-mgarber · 2021-09-22T12:15:18Z

@ikq if you don't mind external contributions i'd be willing to take a swing at writing unit tests for this project's test framework

ikq · 2021-09-22T12:29:09Z

if you don't mind external contributions i'd be willing to take a swing at writing unit tests for this project's test framework

Thank you, I will gladly accept your help.

I have provided an external test for 2 browsers: one is sending an SUBSCRIBE and the other is receiving it and sending a NOTIFY.

Seems in unit test should be simulated 2 instances of JsSIP one send/other receive.
[I don't know how it can be written.
Probably instead two JsSIP instances, can be used single that send the message itself ?]
;-)

stefang42 · 2021-09-22T13:04:27Z

Thanks.
It can be improved (use of Configuration.contact_uri)

P.S.
Usage Configuration.contact_uri will modify default SUBSCRIBE contact - will be missed "+sip.instance" (that I add to default SUBSCRIBE Contact)
User still can set "+sip.instance" to SUBSCRIBE Contact via custom Contact header.

@ikq Is there a specific reason to use Configuration.contact_uri? Why not just do it as in RTCSession, with this._ua.contact.toString? Then it would also use the gruu if one was provided. Or am I missing something?

ikq · 2021-09-23T16:39:24Z

Thank you markusatm for founding the problem.
Thank you stefang42, I use your fix to build Contact in subscriber and notifier.

lib/Notifier.js

jmillan · 2021-09-24T10:53:40Z

Hi,

Yes, in order for this PR to be merged we need tests in JsSIP project.

You don't need two JsSIP instances, just one instance and you should inject a transport that you control (a FAKE transport) that you use to send JsSIP UA SUBSCRIBE|NOTIFY messages and receive and check what comes from JsSIP.

If you need some help please let us know.

ikq · 2021-09-25T20:52:23Z

Hi,
I added nodeunit test to test subscriber notifier communication.

Implemented loopSocket that send message itself, but modified Call-ID in each leg.
(without the modification method findDialog confuses SUBSCRIBE dialog with NOTIFY dialog)
subscriber and notifier run in the same JsSIP instance and send/receive SIP messages.

The test SIP sequence is:

SUBSCRIBE
SUBSCRIBE OK
NOTIFY
NOTIFY OK
unSUBSCRIBE (with expires 0)
unSUBSCRIBE OK
final NOTIFY (with Subscription-State: terminated)
final NOTIFY OK.

The unit test work silently, but if uncomment line JsSIP.debug.enable('JsSIP:*');
can be seen complete SIP communication.

SUBSCRIBE and NOTIFY headers checking:
url, method, contact, subscribe accept, notify subscription-state, content-type and body.
Checked order of event sequence.

devoxy1 · 2023-12-05T15:16:03Z

Up!

ddyner · 2023-12-10T11:23:27Z

Hello. We also want to use this feature. Will this pull request be merged?

jmillan · 2023-12-11T13:34:35Z

This is waiting for the docs to be generated. @ikq are you willing to create a PR for the doc? We cannot merge the feature without it.

ikq · 2023-12-11T14:54:41Z

Writing documentation is never easy for me but let's try.

How do you do this?

I have to download the zip (or git clone) from https://github.com/versatica/jssip.net,

install nanoc and modify content/documentation/api/ua.html

And then check the result using nanoc compile, nanoc view?

If the modified doc compiled, make Pull Request of https://github.com/versatica/jssip.net with

modified api/ua.html,
and created api.subscriber.html, api.notifier.html

Do I understand correctly ?

ikq · 2023-12-17T18:18:42Z

I created pull request for https://github.com/versatica/jssip.net
Please take a look.

devoxy1 · 2024-01-09T09:06:54Z

@jmillan Hello! Could you please check it up? Thank you!

jmillan · 2024-01-09T09:22:06Z

I'll review when I can. If you are in a hurry, please do use this branch freely.

jmillan · 2024-01-09T09:23:20Z

I'll review when I can. If you are in a hurry, please do use this branch freely.

This way you will also collaborate on using, testing and reporting.

devoxy1 · 2024-01-09T11:32:34Z

@jmillan We already using it this way :) but it's a bit difficult to maintain separate branch.
Anyways, thanks you!

jmillan · 2024-01-18T10:25:40Z

I'm not forgetting about this...,

devoxy1 · 2024-03-28T13:03:21Z

@jmillan Hello! Any chance you had time to review this one?
Thanks!

orgads · 2024-07-23T14:16:29Z

@jmillan Ping?

jmillan · 2024-07-29T10:32:25Z

For those willing to use this feature I would suggest you use this branch. I don't have at the moment the resources to re-check and merge this, so please, do use this branch for the time being.

johnmarkovalo · 2024-08-09T10:55:53Z

Looking forward with this merge also 👍

Catlike14 · 2025-08-06T10:22:19Z

@jmillan ping?

orgads · 2025-08-13T06:51:55Z

Pushed a rebased version in #922.

jmillan · 2025-12-19T16:23:09Z

lib/Notifier.js

+   * @param {String} body Notify body
+   * @param {Array<string>} extraHeaders
+   */
+  _notify(subsStateParameters, body=null, extraHeaders=null)


private methods must appear after all public ones. Also, this could be called _sendNotify() also to dissambiguate.

jmillan · 2025-12-19T16:25:02Z

lib/Notifier.js

+  /**
+   * Private API
+   */
+  _dialogTerminated(termination_code)


I think terminate() would be a better name for this, as it is what the method does. RTCSession has the same name for such function.

I see terminate() is already taken, then terminateDialog() would work.

jmillan · 2025-12-19T16:29:12Z

@ikq thanks for the patience. I'll try 🤞 to finish the review and accept it in the following weeks. Can I ask you to not include any change in dist/ folder nor in package.json and to sync the branch with master?

jmillan · 2025-12-19T16:34:30Z

lib/Config.js

-  registrar_server : null,
+  register                : true,
+  register_expires        : 600,
+  register_from_tag_trail : '',


Everything related to register_from_tag_trail does not belong to this PR. It's already in master. Please remove it.

jmillan · 2025-12-19T16:34:40Z

lib/Config.js

+    register_from_tag_trail(register_from_tag_trail)
+    {
+      if (typeof register_from_tag_trail === 'function')
+      {
+        return register_from_tag_trail;
+      }
+
+      return String(register_from_tag_trail);
+    },
+


to be removed.

jmillan · 2025-12-19T16:35:08Z

lib/Registrator.js

+    let fromTag = Utils.newTag();
+
+    if (this._ua.configuration.register_from_tag_trail)
+    {
+      if (typeof this._ua.configuration.register_from_tag_trail === 'function')
+      {
+        fromTag += this._ua.configuration.register_from_tag_trail();
+      }
+      else
+      {
+        fromTag += this._ua.configuration.register_from_tag_trail;
+      }
+    }
+
    const request = new SIPMessage.OutgoingRequest(
      JsSIP_C.REGISTER, this._registrar, this._ua, {
-        'to_uri'  : this._to_uri,
-        'call_id' : this._call_id,
-        'cseq'    : (this._cseq += 1)
+        'to_uri'   : this._to_uri,
+        'call_id'  : this._call_id,
+        'cseq'     : (this._cseq += 1),
+        'from_tag' : fromTag


to be removed.

jmillan · 2025-12-19T16:35:18Z

lib/UA.d.ts

  ha1?: string;
  register?: boolean;
  register_expires?: number;
+  register_from_tag_trail?: string | function() : string;


to be removed.

jmillan · 2025-12-19T16:36:01Z

Also please do not edit CHANGELOG.md

jmillan · 2025-12-19T16:41:04Z

lib/RTCSession.js

+            if (finished)
+            {
+              return;
+            }
+


This change does not belong to this PR. It's already in master. Please remove.

jmillan · 2025-12-19T16:41:26Z

lib/RTCSession.js

            }
-
-            else if (!finished)
+            else


Change already in master, please remove.

jmillan · 2025-12-19T16:41:33Z

lib/RTCSession.js

          connection.addEventListener('icegatheringstatechange', iceGatheringStateListener = () =>
          {
-            if ((connection.iceGatheringState === 'complete') && !finished)
+            if (connection.iceGatheringState === 'complete')


Change already in master, please remove.

jmillan · 2025-12-19T16:43:37Z

lib/Notifier.js

+  /**
+   * Get dialog state.
+   */
+  get state()


Getters are set after constructor. Please check RTCSession.js

orgads · 2025-12-25T11:00:04Z

@ikq thanks for the patience. I'll try 🤞 to finish the review and accept it in the following weeks. Can I ask you to not include any change in dist/ folder nor in package.json and to sync the branch with master?

Done

orgads · 2025-12-25T11:02:17Z

Hi @jmillan. Thanks for the review.

I work with @ikq at Audiocodes, and he agreed to let me take over this change as he has no time to address all the comments.

I pushed a new rebased revision, hopefully addressing all your comments. Please review.

jmillan · 2025-12-25T11:16:21Z

Thanks, I'll take some time on a further, and hopefully last review.

jmillan reviewed Jun 1, 2021

View reviewed changes

ikq requested a review from jmillan August 3, 2021 15:49

mitre-mgarber mentioned this pull request Aug 19, 2021

Added RFC3842 - Message Waiting functionality #731

Closed

jmillan reviewed Sep 24, 2021

View reviewed changes

lib/Notifier.js Show resolved Hide resolved

orgads mentioned this pull request Aug 13, 2025

Subscribe support #922

Closed

jmillan reviewed Dec 19, 2025

View reviewed changes

Subscribe support

d390a7c

orgads force-pushed the subscribe_support branch from b2ef69c to d390a7c Compare December 25, 2025 10:55

Subscribe support #711

Are you sure you want to change the base?

Subscribe support #711

Uh oh!

Conversation

ikq commented May 20, 2021

Uh oh!

jmillan commented May 21, 2021

Uh oh!

jmillan commented May 25, 2021

Uh oh!

jmillan commented May 27, 2021

Uh oh!

jmillan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ikq commented Jun 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmillan commented Jun 8, 2021

Uh oh!

jmillan commented Jun 22, 2021

Uh oh!

ikq commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmillan commented Jun 22, 2021

Uh oh!

ikq commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ikq commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ikq commented Jul 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ikq commented Aug 8, 2021

Uh oh!

jmillan commented Aug 18, 2021

Uh oh!

ikq commented Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmillan commented Aug 23, 2021

Uh oh!

N4COM commented Sep 22, 2021

Uh oh!

ikq commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markusatm commented Sep 22, 2021

Uh oh!

N4COM commented Sep 22, 2021

Uh oh!

ikq commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mitre-mgarber commented Sep 22, 2021

Uh oh!

ikq commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stefang42 commented Sep 22, 2021

Uh oh!

ikq commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jmillan commented Sep 24, 2021

ikq commented Jun 5, 2021 •

edited

Loading

ikq commented Jun 22, 2021 •

edited

Loading

ikq commented Jun 24, 2021 •

edited

Loading

ikq commented Jul 7, 2021 •

edited

Loading

ikq commented Jul 10, 2021 •

edited

Loading

ikq commented Aug 20, 2021 •

edited

Loading

ikq commented Sep 22, 2021 •

edited

Loading

ikq commented Sep 22, 2021 •

edited

Loading

ikq commented Sep 22, 2021 •

edited

Loading

ikq commented Sep 23, 2021 •

edited

Loading

ikq commented Sep 25, 2021 •

edited

Loading

ikq commented Dec 11, 2023 •

edited

Loading

jmillan commented Dec 19, 2025 •

edited

Loading